Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-15410: Delete records with tiered storage integration test (4/4) #14330

Merged
merged 8 commits into from Sep 7, 2023
Merged

KAFKA-15410: Delete records with tiered storage integration test (4/4) #14330

merged 8 commits into from Sep 7, 2023

Conversation

kamalcph
Copy link
Collaborator

@kamalcph kamalcph commented Sep 3, 2023 •
edited

Added the integration test for DELETE_RECORDS API for tiered storage enabled topic.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@kamalcph kamalcph changed the title KAFKA-15410: Delete segments due to log-start-offset breach integration test (4/4) KAFKA-15410: Delete records API with tiered storage integration test (4/4) Sep 3, 2023
@kamalcph kamalcph changed the title KAFKA-15410: Delete records API with tiered storage integration test (4/4) KAFKA-15410: Delete records with tiered storage integration test (4/4) Sep 3, 2023
@satishd satishd added the tiered-storage Pull requests associated with KIP-405 (Tiered Storage) label Sep 4, 2023
@kamalcph kamalcph force-pushed the KAFKA-15410c branch 2 times, most recently from 5117f77 to a6ba58a Compare September 5, 2023 10:08
@kamalcph kamalcph marked this pull request as ready for review September 5, 2023 10:09
@kamalcph
Copy link
Collaborator Author

kamalcph commented Sep 5, 2023

@clolov @divijvaidya @showuon @abhijeetk88 @satishd

Call for review. Please take a look when you get chance!

satishd
satishd reviewed Sep 6, 2023
View reviewed changes
Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kamalcph for the PR. Left a few comments.

core/src/main/java/kafka/log/remote/RemoteLogManager.java Show resolved Hide resolved
core/src/main/java/kafka/log/remote/RemoteLogManager.java Show resolved Hide resolved
core/src/main/java/kafka/log/remote/RemoteLogManager.java Outdated Show resolved Hide resolved
...org/apache/kafka/tiered/storage/integration/DeleteSegmentsDueToLogStartOffsetBreachTest.java Show resolved Hide resolved
@kamalcph
Copy link
Collaborator Author

kamalcph commented Sep 6, 2023

@showuon @satishd

Addressed your review comments. Please take another look.

clolov
clolov approved these changes Sep 6, 2023
View reviewed changes
Copy link
Collaborator

@clolov clolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes, they make sense to me!

...org/apache/kafka/tiered/storage/integration/DeleteSegmentsDueToLogStartOffsetBreachTest.java Outdated Show resolved Hide resolved
showuon
showuon approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@showuon showuon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just one minor comment. Thanks for adding the tests.

core/src/main/java/kafka/log/remote/RemoteLogManager.java Show resolved Hide resolved
satishd
satishd reviewed Sep 7, 2023
View reviewed changes
Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kamalcph for addressing the review comments. I have one last comment.

core/src/main/java/kafka/log/remote/RemoteLogManager.java Outdated
return false;
});
if (isSegmentDeleted) {
remainingBreachedSize = retentionSizeData.isPresent() ?
Copy link
Member

@satishd satishd Sep 7, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we should not change remainingBreachedSize as that is computed only with the segments that are in the current leader epoch lineage. This is the change/fix that you had putup in earlier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Addressed your review comment. PTAL.

satishd
satishd approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kamalcph for addressing the review comments. LGTM.

@satishd
Copy link
Member

satishd commented Sep 7, 2023

There are a few test failures unrelated to this change. Merging it to trunk and 3.6.

@satishd satishd merged commit 6e818c6 into apache:trunk Sep 7, 2023
1 check failed
@kamalcph kamalcph deleted the KAFKA-15410c branch September 7, 2023 15:50
satishd pushed a commit that referenced this pull request Sep 7, 2023
@kamalcph @satishd
KAFKA-15410: Delete records with tiered storage integration test (4/4) (
946ab8f 
#14330)

* Added the integration test for DELETE_RECORDS API for tiered storage enabled topic
* Added validation checks before removing remote log segments for log-start-offset breach

Reviewers: Satish Duggana <satishd@apache.org>, Luke Chen <showuon@gmail.com>, Christo Lolov <lolovc@amazon.com>
@divijvaidya
Copy link
Contributor

Folks the test introduced in this PR has been flaky lately - https://ge.apache.org/scans/tests?search.rootProjectNames=kafka&search.timeZoneId=Europe/Berlin&tests.container=org.apache.kafka.tiered.storage.integration.DeleteSegmentsDueToLogStartOffsetBreachTest&tests.test=executeTieredStorageTest(String)%5B2%5D

CI link - https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14365/2/testReport/junit/org.apache.kafka.tiered.storage.integration/DeleteSegmentsDueToLogStartOffsetBreachTest/Build___JDK_17_and_Scala_2_13___executeTieredStorageTest_String__quorum_kraft/

@kamalcph
Copy link
Collaborator Author

Folks the test introduced in this PR has been flaky lately - https://ge.apache.org/scans/tests?search.rootProjectNames=kafka&search.timeZoneId=Europe/Berlin&tests.container=org.apache.kafka.tiered.storage.integration.DeleteSegmentsDueToLogStartOffsetBreachTest&tests.test=executeTieredStorageTest(String)%5B2%5D

CI link - https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14365/2/testReport/junit/org.apache.kafka.tiered.storage.integration/DeleteSegmentsDueToLogStartOffsetBreachTest/Build___JDK_17_and_Scala_2_13___executeTieredStorageTest_String__quorum_kraft/

Will look into it later today.

@kamalcph
Copy link
Collaborator Author

Folks the test introduced in this PR has been flaky lately - https://ge.apache.org/scans/tests?search.rootProjectNames=kafka&search.timeZoneId=Europe/Berlin&tests.container=org.apache.kafka.tiered.storage.integration.DeleteSegmentsDueToLogStartOffsetBreachTest&tests.test=executeTieredStorageTest(String)%5B2%5D
CI link - https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-14365/2/testReport/junit/org.apache.kafka.tiered.storage.integration/DeleteSegmentsDueToLogStartOffsetBreachTest/Build___JDK_17_and_Scala_2_13___executeTieredStorageTest_String__quorum_kraft/

Will look into it later today.

Opened #14439 to fix this flaky test.

mjsax pushed a commit to confluentinc/kafka that referenced this pull request Nov 22, 2023
@kamalcph @mjsax
KAFKA-15410: Delete records with tiered storage integration test (4/4) (
9c9e1c7 
apache#14330)

* Added the integration test for DELETE_RECORDS API for tiered storage enabled topic
* Added validation checks before removing remote log segments for log-start-offset breach

Reviewers: Satish Duggana <satishd@apache.org>, Luke Chen <showuon@gmail.com>, Christo Lolov <lolovc@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@satishd satishd satishd approved these changes

@clolov clolov clolov approved these changes

@showuon showuon showuon approved these changes

Assignees
No one assigned
Labels
tiered-storage Pull requests associated with KIP-405 (Tiered Storage)
Projects
None yet
Milestone
No milestone
5 participants
@kamalcph @satishd @divijvaidya @clolov @showuon