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.

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

Fix the bug where uploaded segments cannot be deleted on real-time table #9579

Merged
merged 1 commit into from
Oct 13, 2022

Conversation

Jackie-Jiang
Copy link
Contributor

Related to #8283

Since we added the support of uploading segments to real-time table, we cannot always derive the table type from the segment name. When the provided table name has the type suffix, do not try to derive the table type from the segment name.

@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2022

Codecov Report

Merging #9579 (8a24225) into master (2d520d4) will increase coverage by 37.82%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##             master    #9579       +/-   ##
=============================================
+ Coverage     26.03%   63.85%   +37.82%     
- Complexity       44     5240     +5196     
=============================================
  Files          1922     1881       -41     
  Lines        102937   100962     -1975     
  Branches      15657    15419      -238     
=============================================
+ Hits          26796    64469    +37673     
+ Misses        73435    31768    -41667     
- Partials       2706     4725     +2019     
Flag Coverage Δ
integration1 ?
unittests1 67.41% <ø> (?)
unittests2 15.60% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ler/api/resources/PinotSegmentRestletResource.java 10.02% <0.00%> (-8.33%) ⬇️
...inot/controller/helix/ControllerRequestClient.java 34.93% <0.00%> (-1.29%) ⬇️
...va/org/apache/pinot/core/routing/RoutingTable.java 0.00% <0.00%> (-100.00%) ⬇️
...va/org/apache/pinot/common/config/NettyConfig.java 0.00% <0.00%> (-100.00%) ⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java 0.00% <0.00%> (-100.00%) ⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/BrokerQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/MinionQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
...ache/pinot/server/access/AccessControlFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...he/pinot/common/messages/TableDeletionMessage.java 0.00% <0.00%> (-100.00%) ⬇️
... and 1565 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

lgtm overall.

one observation is ControllerRequestClient doesn't have unit test. good to add one

Comment on lines +93 to +106
// Test dropping all segments one by one
List<String> segments = listSegments(realtimeTableName);
assertFalse(segments.isEmpty());
for (String segment : segments) {
dropSegment(realtimeTableName, segment);
}
// NOTE: There is a delay to remove the segment from property store
TestUtils.waitForCondition((aVoid) -> {
try {
return listSegments(realtimeTableName).isEmpty();
} catch (IOException e) {
throw new RuntimeException(e);
}
}, 60_000L, "Failed to drop the segments");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, why is this test during tearDown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the tear-down process. OfflineClusterIntegrationTest has similar test. We may consider adding dependency to the test so that we can move it into a test, but prefer doing it separately

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah good enough for now we can follow up

@Jackie-Jiang Jackie-Jiang merged commit 97bff59 into apache:master Oct 13, 2022
@Jackie-Jiang Jackie-Jiang deleted the fix_delete_segment_api branch October 13, 2022 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants