You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "kamalcph (via GitHub)" <gi...@apache.org> on 2023/09/19 17:31:34 UTC
[GitHub] [kafka] kamalcph opened a new pull request, #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
kamalcph opened a new pull request, #14407:
URL: https://github.com/apache/kafka/pull/14407
When a remote log segment contains multiple epoch, then it gets considered for multiple times during breach by retention size/time/start-offset. This will affect the deletion by remote log retention size as it deletes the number of segments lesser than expected. This is a follow-up of KAFKA-15352
### 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kamalcph commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1726203068
@clolov @divijvaidya @showuon @satishd
Call for review. 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1731082887
I will wait for other reviewers on this PR (cc: @satishd @clolov) to provide an approval in the next few days before merging this.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kamalcph commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1726278190
> I had a comment to fix this/similar problem in original PR - [#13561 (comment)](https://github.com/apache/kafka/pull/13561#discussion_r1293305304)
>
> I am curious, why didn't the test which was added to resolve the comment fail? Is it because the test only checked for eligibility of a segment for calculation of size and didn't actually check if same segment is being counted twice?
We don't have a test for the scenario mentioned in the [comment](https://github.com/apache/kafka/pull/13561#discussion_r1293305304). And, there was a regression after #14349. I have added couple of tests to verify the deleted segment count and log-start-offset.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] clolov commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "clolov (via GitHub)" <gi...@apache.org>.
clolov commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1727495187
I will review this in a couple of hours, apologies for the delay
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kamalcph commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1730747471
@clolov @divijvaidya @showuon @satishd
Test failures are unrelated. Please take another look!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kamalcph commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1728867925
> @kamalcph TS related tests are failing such as https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14407/2/testReport/junit/org.apache.kafka.server.log.remote.metadata.storage/TopicBasedRemoteLogMetadataManagerTest/Build___JDK_20_and_Scala_2_13___testRemoteLogSizeCalculationWithSegmentsOfDifferentEpochs__/
>
> Please check.
The test failures are unrelated to this PR.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1727496623
@kamalcph TS related tests are failing such as https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14407/2/testReport/junit/org.apache.kafka.server.log.remote.metadata.storage/TopicBasedRemoteLogMetadataManagerTest/Build___JDK_20_and_Scala_2_13___testRemoteLogSizeCalculationWithSegmentsOfDifferentEpochs__/
Please check.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishd commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1733562853
Merging it to trunk as the failed tests are not related to the changes introduced in this PR.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] satishd merged pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "satishd (via GitHub)" <gi...@apache.org>.
satishd merged PR #14407:
URL: https://github.com/apache/kafka/pull/14407
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1726259952
I had a comment to fix this/similar problem in original PR - https://github.com/apache/kafka/pull/13561#discussion_r1293305304
I am curious, why didn't the test which was added to resolve the comment fail? Is it because the test only checked for eligibility of a segment for calculation of size and didn't actually check if same segment is being counted twice?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] kamalcph commented on a diff in pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on code in PR #14407:
URL: https://github.com/apache/kafka/pull/14407#discussion_r1330492380
##########
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##########
@@ -993,7 +988,9 @@ private void cleanupExpiredRemoteLogSegments() throws RemoteStorageException, Ex
return;
}
RemoteLogSegmentMetadata metadata = segmentsIterator.next();
-
+ if (segmentsToDelete.contains(metadata)) {
Review Comment:
This is the main fix.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #14407: KAFKA-15479: Remote log segments should be considered once for retention breach
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #14407:
URL: https://github.com/apache/kafka/pull/14407#issuecomment-1733343995
@satishd do you have any additional comments on this PR? @clolov (the other reviewer in this PR) has already approved and we are waiting for you.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org