Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
showuon merged PR #15748: URL: https://github.com/apache/kafka/pull/15748 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
showuon commented on PR #15748: URL: https://github.com/apache/kafka/pull/15748#issuecomment-2073928078 Failed tests are unrelated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on code in PR #15748: URL: https://github.com/apache/kafka/pull/15748#discussion_r1576190364 ## core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala: ## @@ -121,9 +121,9 @@ class LogCleanerParameterizedIntegrationTest extends AbstractLogCleanerIntegrati // Set the last modified time to an old value to force deletion of old segments val endOffset = log.logEndOffset log.logSegments.foreach(_.lastModified = time.milliseconds - (2 * retentionMs)) -TestUtils.waitUntilTrue(() => log.logStartOffset == endOffset, +TestUtils.waitUntilTrue(() => 1 == log.numberOfSegments, "Timed out waiting for deletion of old segments") -assertEquals(1, log.numberOfSegments) +assertEquals(log.logStartOffset, endOffset) Review Comment: Raised #15787 PR to fix it in trunk -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on code in PR #15748: URL: https://github.com/apache/kafka/pull/15748#discussion_r1576175394 ## core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala: ## @@ -121,9 +121,9 @@ class LogCleanerParameterizedIntegrationTest extends AbstractLogCleanerIntegrati // Set the last modified time to an old value to force deletion of old segments val endOffset = log.logEndOffset log.logSegments.foreach(_.lastModified = time.milliseconds - (2 * retentionMs)) -TestUtils.waitUntilTrue(() => log.logStartOffset == endOffset, +TestUtils.waitUntilTrue(() => 1 == log.numberOfSegments, "Timed out waiting for deletion of old segments") -assertEquals(1, log.numberOfSegments) +assertEquals(log.logStartOffset, endOffset) Review Comment: The test is flaky post landing the PR in trunk: https://ge.apache.org/scans/tests?search.relativeStartTime=P28D=kafka=Europe%2FBerlin=kafka.log.LogCleanerParameterizedIntegrationTest We have to raise a similar fix in trunk. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on code in PR #15748: URL: https://github.com/apache/kafka/pull/15748#discussion_r1576175394 ## core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala: ## @@ -121,9 +121,9 @@ class LogCleanerParameterizedIntegrationTest extends AbstractLogCleanerIntegrati // Set the last modified time to an old value to force deletion of old segments val endOffset = log.logEndOffset log.logSegments.foreach(_.lastModified = time.milliseconds - (2 * retentionMs)) -TestUtils.waitUntilTrue(() => log.logStartOffset == endOffset, +TestUtils.waitUntilTrue(() => 1 == log.numberOfSegments, "Timed out waiting for deletion of old segments") -assertEquals(1, log.numberOfSegments) +assertEquals(log.logStartOffset, endOffset) Review Comment: The tests is flaky post landing the PR in trunk: https://ge.apache.org/scans/tests?search.relativeStartTime=P28D=kafka=Europe%2FBerlin=kafka.log.LogCleanerParameterizedIntegrationTest We have to raise a similar fix in trunk. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
showuon commented on code in PR #15748: URL: https://github.com/apache/kafka/pull/15748#discussion_r1575561675 ## core/src/test/scala/unit/kafka/log/LogCleanerParameterizedIntegrationTest.scala: ## @@ -121,9 +121,9 @@ class LogCleanerParameterizedIntegrationTest extends AbstractLogCleanerIntegrati // Set the last modified time to an old value to force deletion of old segments val endOffset = log.logEndOffset log.logSegments.foreach(_.lastModified = time.milliseconds - (2 * retentionMs)) -TestUtils.waitUntilTrue(() => log.logStartOffset == endOffset, +TestUtils.waitUntilTrue(() => 1 == log.numberOfSegments, "Timed out waiting for deletion of old segments") -assertEquals(1, log.numberOfSegments) +assertEquals(log.logStartOffset, endOffset) Review Comment: I'm re-triggering the CI since there are 2 jobs not finished. Meanwhile, could you help explain why we need these 2 changes in v3.6, but not in v3.7/trunk? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on PR #15748: URL: https://github.com/apache/kafka/pull/15748#issuecomment-2071274526 @showuon Test failures are fixed! Please take another look. Let me know if we have to fix the tests in 3.7 and trunk (but they weren't flaky). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
showuon commented on PR #15748: URL: https://github.com/apache/kafka/pull/15748#issuecomment-2063302914 @kamalcph , the tests are still failing: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15748/ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
showuon merged PR #15747: URL: https://github.com/apache/kafka/pull/15747 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
showuon commented on PR #15747: URL: https://github.com/apache/kafka/pull/15747#issuecomment-2062884304 Failed tests are unrelated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
showuon commented on PR #15748: URL: https://github.com/apache/kafka/pull/15748#issuecomment-2062882937 Looks like `LogCleanerParameterizedIntegrationTest` failed due to this change. Please take a look. Thanks. https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15748/1/#showFailuresLink -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2062050733 > @kamalcph : Do you want this PR to be in 3.7 and 3.6? If so, could you create a separate cherry-pick RP? Thanks. Opened #15747 and #15748 to backport it to 3.7 and 3.6 branches. PTAL. Thanks for the review! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph opened a new pull request, #15748: URL: https://github.com/apache/kafka/pull/15748 cherry-picked from: d092787487047178c674bd60a182129a8d997c4a Co-authored-by: hzh0425 <642256...@qq.com> ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph opened a new pull request, #15747: URL: https://github.com/apache/kafka/pull/15747 cherry-picked from: d092787487047178c674bd60a182129a8d997c4a Co-authored-by: hzh0425 <642256...@qq.com> ### Committer Checklist (excluded from commit message) - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
junrao commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2061739168 @kamalcph : Do you want this PR to be in 3.7 and 3.6? If so, could you create a separate cherry-pick RP? Thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
junrao merged PR #15631: URL: https://github.com/apache/kafka/pull/15631 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
junrao commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2061732009 @kamalcph : Thanks for triaging the failed tests. The PR LGTM. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2060333550 @junrao Gentle reminder. PTAL. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2053698624 Re-triggered the tests. Tests failures are unrelated. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
junrao commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2052208460 @kamalcph : Thanks for the update. JDK 11 and Scala 2.13 didn't complete in the last test run. Could you trigger another run of the tests? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2050932821 @junrao Please take another look, thanks! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2046510235 Rebased the PR with trunk, the test failures are unrelated. Ran the 11 failed tests locally, all of them got succeeded: ``` ./gradlew cleanTest core:test --tests kafka.log.LogCleanerParameterizedIntegrationTest.testCleansCombinedCompactAndDeleteTopic tools:test --tests org.apache.kafka.tools.consumer.group.DescribeConsumerGroupTest.testDescribeExistingGroupWithNoMembers trogdor:test --tests org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated connect:mirror:test --tests org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplicateSourceDefault core:test --tests kafka.api.ConsumerBounceTest.testConsumptionWithBrokerFailures --tests kafka.api.PlaintextAdminIntegrationTest.testElectPreferredLeaders --tests kafka.server.DelegationTokenRequestsTest.testDelegationTokenRequests --tests kafka.zk.ZkMigrationIntegrationTest.testPartitionReassignmentInHybridMode tools:test --tests org.apache.kafka.tools.MetadataQuorumCommandTest.testDescribeQuorumStatusSuccessful --tests org.apache.kafka.tools.reassign.ReassignPartitionsIntegrationTest.testPr oduceAndConsumeWithReassignmentInProgress trogdor:test --tests org.apache.kafka.trogdor.coordinator.CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
junrao commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2045573518 @kamalcph : Thanks for the updated PR. The code looks good to me. Do you know why there were 106 failed tests? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2044326621 @junrao Addressed the review comments. PTAL. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
junrao commented on code in PR #15631:
URL: https://github.com/apache/kafka/pull/15631#discussion_r1556331481
##
core/src/test/scala/unit/kafka/log/UnifiedLogTest.scala:
##
@@ -952,8 +952,9 @@ class UnifiedLogTest {
assertEquals(0, lastSeq)
}
- @Test
- def testRetentionDeletesProducerStateSnapshots(): Unit = {
+ @ParameterizedTest(name = "testRetentionDeletesProducerStateSnapshots with
empty-active-segment: {0}")
+ @ValueSource(booleans = Array(true, false))
+ def testRetentionDeletesProducerStateSnapshots(isEmptyActiveSegment:
Boolean): Unit = {
Review Comment:
isEmptyActiveSegment => createEmptyActiveSegment and change the test name
accordingly.
##
core/src/main/scala/kafka/log/UnifiedLog.scala:
##
@@ -1532,10 +1532,14 @@ class UnifiedLog(@volatile var logStartOffset: Long,
}
}
localLog.checkIfMemoryMappedBufferClosed()
-// remove the segments for lookups
-localLog.removeAndDeleteSegments(segmentsToDelete, asyncDelete = true,
reason)
+if (segmentsToDelete.nonEmpty) {
+ // increment the local-log-start-offset before removing the segment
for lookups
Review Comment:
We probably should say "increment local-log-start-offset or
log-start-offset" since `incrementStartOffset()` could update either one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16073: Increment the local-log-start-offset before deleting segments in memory table [kafka]
kamalcph commented on PR #15631: URL: https://github.com/apache/kafka/pull/15631#issuecomment-2028316319 Unit tests are not added to assert that the as the local-log-start-offset is incremented before the segments are removed from inmemory table as it is in private method. PTAL. cc @satishd @junrao @showuon -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
