Add a Server API to list segments that need to be refreshed for a table #14451

vrajat · 2024-11-14T10:24:30Z

The core feature in this PR is a function BaseTableDataManager.getSegmentsForRefresh which finds all the segments of a table that requires a refresh. It accepts a Table Config and Schema and compares that with the current state of a segment.

This functionality will be used by other components such as minion tasks to decide which segments have to be refreshed.

The function is exposed by the following server API:

/tables/{tableName}/segments/needRefresh

Response is:
[
  {
     "segmentName":"...",
     "reason":"..."
  },
...
]

segmentName is the name of the segment.
reason contains a simple message for why the segment needs to be refreshed.

There is also a controller API that consolidates the responses from all servers.

/segments/orders_OFFLINE/needRefresh

Response is:
{
  "<server name>" : [{
     "segmentName":"...",
     "reason":"..."
  }],
...
]

Example:

{
  "http://192.168.0.111:7500/tables/orders_OFFLINE/segments/needRefresh__1": [
    {
      "segmentName": "orders_OFFLINE_0",
      "reason": "column added"
    }
  ]
}

Fix for #14450

yashmayya · 2024-11-15T04:30:19Z

What's the difference between this and #13789?

vrajat · 2024-11-15T04:44:51Z

This API is for refresh while IIUC that PR is for reload. I should make it clear that this PR is for refresh in the title.

…

On Fri, Nov 15, 2024 at 10:00 AM Yash Mayya ***@***.***> wrote: What's the difference between this and #13789 <#13789>? — Reply to this email directly, view it on GitHub <#14451 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMP7GRJ3H2O3PHX2PI2CD32AV2HDAVCNFSM6AAAAABRYRMF3KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINZXHEZTSNRXGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

codecov-commenter · 2024-11-19T05:05:25Z

Codecov Report

Attention: Patch coverage is 49.74619% with 99 lines in your changes missing coverage. Please review.

Project coverage is 63.82%. Comparing base (59551e4) to head (621a816).
Report is 1383 commits behind head on master.

Files with missing lines	Patch %	Lines
.../pinot/core/data/manager/BaseTableDataManager.java	66.42%	37 Missing and 9 partials ⚠️
...t/controller/util/ServerSegmentMetadataReader.java	0.00%	25 Missing ⚠️
...ler/api/resources/PinotSegmentRestletResource.java	0.00%	9 Missing ⚠️
...che/pinot/controller/util/TableMetadataReader.java	0.00%	6 Missing ⚠️
...egment/local/data/manager/NeedRefreshResponse.java	53.84%	6 Missing ⚠️
...che/pinot/server/api/resources/TablesResource.java	0.00%	6 Missing ⚠️
...spi/utils/builder/ControllerRequestURLBuilder.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14451      +/-   ##
============================================
+ Coverage     61.75%   63.82%   +2.07%     
- Complexity      207     1570    +1363     
============================================
  Files          2436     2674     +238     
  Lines        133233   146983   +13750     
  Branches      20636    22548    +1912     
============================================
+ Hits          82274    93816   +11542     
- Misses        44911    46224    +1313     
- Partials       6048     6943     +895

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.80% <49.74%> (+2.10%)`	⬆️
java-21	`63.71% <49.74%> (+2.09%)`	⬆️
skip-bytebuffers-false	`63.81% <49.74%> (+2.07%)`	⬆️
skip-bytebuffers-true	`63.69% <49.74%> (+35.97%)`	⬆️
temurin	`63.82% <49.74%> (+2.07%)`	⬆️
unittests	`63.82% <49.74%> (+2.07%)`	⬆️
unittests1	`55.60% <64.90%> (+8.71%)`	⬆️
unittests2	`34.53% <0.00%> (+6.79%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

KKcorps · 2024-11-22T12:27:21Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+      }
+
+      if (Objects.isNull(source.getH3Index()) == h3Indexes.contains(columnName)) {
+        LOGGER.debug("tableNameWithType: {}, segmentName: {}, change: h3 index changed", tableNameWithType,


nit: Can we improve this log to also have columnName for which the index was changed

Added columnName to all debug logs

KKcorps · 2024-11-22T13:26:50Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+  @Override
+  public List<NeedRefreshResponse> getSegmentsForRefresh(TableConfig tableConfig, Schema schema) {
+    List<NeedRefreshResponse> segmentsRequiringRefresh = new ArrayList<>();
+    List<SegmentDataManager> segmentDataManagers = acquireAllSegments();


acquireAllSegments also increases the ref count
so we need to decrease it as well once we get the list

Also, since we are running a for loop, wouldn't a better way be to acquire each segment one by one

I am releasing the segments now. To acquire a segment, how do I get the segment name ? One option is to acquire all segments, convert that into a list of names and then reacquire one by one. Any other options ?

Good point about acquireAll. Yes thats one way. If we have a large table/with a large number of segments, this check can take time. However by the time you get to the segment, that can be released? I guess acquireSegment will return null in that case.

swaminathanmanish · 2024-11-24T11:25:01Z

...t-controller/src/main/java/org/apache/pinot/controller/util/ServerSegmentMetadataReader.java

+      }
+    }
+    if (failedParses != 0) {
+      LOGGER.error("Unable to parse server {} / {} response due to an error: ", failedParses, serverURLs.size());


Please add table name to this msg

swaminathanmanish · 2024-11-24T11:25:06Z

...t-controller/src/main/java/org/apache/pinot/controller/util/ServerSegmentMetadataReader.java

+                new TypeReference<List<NeedRefreshResponse>>() { }));
+      } catch (Exception e) {
+        failedParses++;
+        LOGGER.error("Unable to parse server {} response due to an error: ", streamResponse.getKey(), e);


Please add table name to this msg

swaminathanmanish · 2024-11-24T11:39:14Z

...ent-local/src/main/java/org/apache/pinot/segment/local/data/manager/NeedRefreshResponse.java

+public class NeedRefreshResponse {
+  private final String _segmentName;
+  private final boolean _needRefresh;
+  private final String _reason;


Good idea to add reason as well !

swaminathanmanish · 2024-11-24T11:43:46Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+  @Override
+  public List<NeedRefreshResponse> getSegmentsForRefresh(TableConfig tableConfig, Schema schema) {
+    List<NeedRefreshResponse> segmentsRequiringRefresh = new ArrayList<>();
+    List<SegmentDataManager> segmentDataManagers = acquireAllSegments();


Good point about acquireAll. Yes thats one way. If we have a large table/with a large number of segments, this check can take time. However by the time you get to the segment, that can be released? I guess acquireSegment will return null in that case.

swaminathanmanish · 2024-11-24T11:59:04Z

...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java

+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Gets a list of segments that need to be refreshed from servers hosting the table", notes =
+      "Gets a list of segments that need to be refreshed from servers hosting the table")
+  public Map<String, List<NeedRefreshResponse>> getTableRefreshMetadata(


How will the user know if we got partial Vs full response.
Would it be useful to find out missing segments for which we never got a response?
After the getSegmentsForRefreshFromServer, we can get the list of segments from ideal state and do a diff. If there's a way to propagate that list, we can just retry that list in the subsequent call.
Or at least the client knows that it did not get a complete list.

vrajat requested review from KKcorps and swaminathanmanish November 14, 2024 10:24

vrajat changed the title ~~Add a server API to list segments out of date with table config~~ Add a Server API to list segments that need to be refreshed for a table Nov 20, 2024

vrajat added feature ingestion rest-api labels Nov 20, 2024

vrajat marked this pull request as ready for review November 21, 2024 07:10

vrajat added 19 commits November 21, 2024 16:00

checkpoint

82e17ab

Create API to get segments needing refresh

da4b8d4

Add controller API

397d107

Checkstyle

47be645

Checkpoint tests

ed9dbb5

Consolidate setup to static variables

3f402c8

Checkstyle

ee33786

Add more index tests

757866a

Partition tests

22ee786

Add NullVectorValue Test

7fe23b3

Add NeedRefreshResponse with reason

b46f5df

Add NeedRefreshResponse with reason

c8dce98

Checkstyle & spotless

130efe2

Fix Jackson serde

11665b5

Fix FST Index test

3dfa453

Checkpoint H3 experiments

aeb6898

Checkpoint tests

4adf49c

Fix all tests

e10c8a0

Doc strings

71cb333

vrajat force-pushed the rv-need-refresh branch from 12b09b3 to 71cb333 Compare November 21, 2024 10:30

KKcorps reviewed Nov 22, 2024

View reviewed changes

vrajat added 2 commits November 22, 2024 21:16

Add column name to debug logs

ddbb85b

Release segments

621a816

xiangfu0 requested review from Jackie-Jiang, shounakmk219 and klsince November 23, 2024 02:03

swaminathanmanish reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Server API to list segments that need to be refreshed for a table #14451

Add a Server API to list segments that need to be refreshed for a table #14451

vrajat commented Nov 14, 2024 •

edited

Loading

yashmayya commented Nov 15, 2024

vrajat commented Nov 15, 2024 via email

codecov-commenter commented Nov 19, 2024 •

edited

Loading

KKcorps Nov 22, 2024

vrajat Nov 22, 2024 •

edited

Loading

KKcorps Nov 22, 2024

KKcorps Nov 22, 2024

vrajat Nov 22, 2024

swaminathanmanish Nov 24, 2024

swaminathanmanish Nov 24, 2024

swaminathanmanish Nov 24, 2024

swaminathanmanish Nov 24, 2024

swaminathanmanish Nov 24, 2024

swaminathanmanish Nov 24, 2024

Add a Server API to list segments that need to be refreshed for a table #14451

Are you sure you want to change the base?

Add a Server API to list segments that need to be refreshed for a table #14451

Conversation

vrajat commented Nov 14, 2024 • edited Loading

yashmayya commented Nov 15, 2024

vrajat commented Nov 15, 2024 via email

codecov-commenter commented Nov 19, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

vrajat Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrajat commented Nov 14, 2024 •

edited

Loading

codecov-commenter commented Nov 19, 2024 •

edited

Loading

vrajat Nov 22, 2024 •

edited

Loading