#12117 Support for Server & Controller API to check for Segments reload of a table in servers #13789

deepthi912 · 2024-08-09T21:18:47Z

Context:
#12117
This PR adds two APIs:
Server API

An API on the server which returns server id and boolean result(to check if any segments need to be reloaded for a given server).
Response for the server API will be something like :
{
"instanceId": "Server_10.0.0.215_7050",
"needReload": false
}

Controller API
2) Controller API which checks if the reload is needed on a table taking into verbose flag consideration:

For example if verbose is true, Response will be like :

{
"needReload": false,
"serverToSegmentsReloadList": {
"Server_10.0.0.215_7050": {
"needReload": false,
"instanceId": "Server_10.0.0.215_7050"
}
}
}

If verbose flag is set to false, serverToSegmentsReloadList map will be empty :

{
"needReload": false,
"serverToSegmentsReloadList": {}
}

Extension: This api can add extra details about the list of segments to be reloaded in the response in the future.

Responses from Controller API:

apache#12117

checkstyle exception

line characters checkstyle

codecov-commenter · 2024-08-09T22:33:53Z

Codecov Report

Attention: Patch coverage is 11.65049% with 91 lines in your changes missing coverage. Please review.

Project coverage is 57.49%. Comparing base (59551e4) to head (aa3a722).
Report is 953 commits behind head on master.

Files with missing lines	Patch %	Lines
...t/controller/util/ServerSegmentMetadataReader.java	0.00%	24 Missing ⚠️
...ler/api/resources/PinotSegmentRestletResource.java	0.00%	22 Missing ⚠️
.../pinot/core/data/manager/BaseTableDataManager.java	0.00%	15 Missing ⚠️
...che/pinot/controller/util/TableMetadataReader.java	0.00%	13 Missing ⚠️
...che/pinot/server/api/resources/TablesResource.java	0.00%	9 Missing ⚠️
...inot/controller/helix/ControllerRequestClient.java	0.00%	5 Missing ⚠️
...spi/utils/builder/ControllerRequestURLBuilder.java	0.00%	2 Missing ⚠️
...l/indexsegment/immutable/ImmutableSegmentImpl.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13789      +/-   ##
============================================
- Coverage     61.75%   57.49%   -4.27%     
+ Complexity      207      197      -10     
============================================
  Files          2436     2582     +146     
  Lines        133233   142544    +9311     
  Branches      20636    22128    +1492     
============================================
- Hits          82274    81949     -325     
- Misses        44911    54100    +9189     
- Partials       6048     6495     +447

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`57.45% <11.65%> (-4.26%)`	⬇️
java-21	`57.38% <11.65%> (-4.25%)`	⬇️
skip-bytebuffers-false	`57.46% <11.65%> (-4.28%)`	⬇️
skip-bytebuffers-true	`57.36% <11.65%> (+29.63%)`	⬆️
temurin	`57.49% <11.65%> (-4.27%)`	⬇️
unittests	`57.48% <11.65%> (-4.27%)`	⬇️
unittests1	`40.21% <40.00%> (-6.68%)`	⬇️
unittests2	`27.81% <0.00%> (+0.08%)`	⬆️

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.

npawar · 2024-08-12T21:34:42Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+      boolean mismatchCheck = false;
+      for (SegmentDataManager segmentDataManager : segmentDataManagers) {
+        Set<String> segmentColumns = SegmentMetadataFetcher.getSegmentColumns(segmentDataManager, columns);
+        if (!segmentColumns.containsAll(schemaColumns)) {


we would need to also check indexing changes. reload will be needed for indexing changes, dictionary changes, along with new column added

I tried to use little detailed check from SegmentPreProcessor.needProcess() to compare the same. But I am doubtful of the amount of time it would take. Do you want me to just compare the indexes map corresponding to each column instead?

which specific part about needPreprocess are you doubtful about?

@npawar We should be good. I was thinking there were some extra config checks happening in needProcess!

segments reload check including indexes metadata

checkstyle unused import removal

remove unused code to get the columns

Jackie-Jiang

The overall direction looks good!

Jackie-Jiang · 2024-08-13T20:56:23Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+  public String checkMismatchedSegments(
+      @ApiParam(value = "Table Name with type", required = true) @PathParam("tableName") String tableName,
+      @ApiParam(value = "Column name", allowMultiple = true) @QueryParam("columns") @DefaultValue("")
+      List<String> columns, @Context HttpHeaders headers) {


Is columns needed?

Not needed, removed

Jackie-Jiang · 2024-08-13T20:58:58Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+    Pair<TableConfig, Schema> tableConfigSchema = tableDataManager.fetchTableConfigAndSchema();
+    IndexLoadingConfig indexLoadingConfig =
+        tableDataManager.getIndexLoadingConfig(tableConfigSchema.getLeft(), tableConfigSchema.getRight());


Suggested change

Pair<TableConfig, Schema> tableConfigSchema = tableDataManager.fetchTableConfigAndSchema();

IndexLoadingConfig indexLoadingConfig =

tableDataManager.getIndexLoadingConfig(tableConfigSchema.getLeft(), tableConfigSchema.getRight());

IndexLoadingConfig indexLoadingConfig = tableDataManager.fetchIndexLoadingConfig();

Jackie-Jiang · 2024-08-13T21:00:20Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+      boolean mismatchCheck = false;
+      for (SegmentDataManager segmentDataManager : segmentDataManagers) {
+        SegmentZKMetadata segmentZKMetadata = tableDataManager.fetchZKMetadata(segmentDataManager.getSegmentName());
+        if (tableDataManager.checkReloadSegment(segmentZKMetadata,


I'd suggest pushing down the logic completely into the TableDataManager so that it is easier to manage and extend in the future. Basically add an API needReload() to the TableDataManager

Jackie-Jiang · 2024-08-13T21:03:06Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+  @GET
+  @Path("/tables/{tableName}/segments/mismatch")
+  @Produces(MediaType.APPLICATION_JSON)
+  @ApiOperation(value = "Checks if there is any mismatch of columns in a segment", notes =


The API path and doc is a little bit misleading. We want to check if reload is needed, instead of checking column mismatch

Jackie-Jiang · 2024-08-13T21:03:41Z

...n/src/main/java/org/apache/pinot/common/restlet/resources/SegmentColumnMismatchResponse.java

+ *
+ * It has details of server id and returns true/false if there are any segments to be reloaded or not.
+ */
+public class SegmentColumnMismatchResponse {


Suggest renaming it to SegmentReloadCheckResponse and rename the fields accordingly

Jackie-Jiang · 2024-08-13T21:03:53Z

...n/src/main/java/org/apache/pinot/common/restlet/resources/SegmentColumnMismatchResponse.java

+  boolean _isMismatch;
+  String _serverInstanceId;


They can be defined as private final

Jackie-Jiang · 2024-08-13T21:06:41Z

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

+
+    // If the segment doesn't exist on server or its CRC has changed, then we
+    // need to fall back to download the segment from deep store to load it.
+    if (segmentMetadata == null || !hasSameCRC(zkMetadata, segmentMetadata)) {


Let's not read ZK metadata or do CRC check here because it can drastically increase the ZK load. needPreprocess() check should be enough

Jackie-Jiang · 2024-08-13T21:07:47Z

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

+      Schema schema = indexLoadingConfig.getSchema();
+      //if the reload of the segment is not needed then return false
+      if (!ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig, schema)) {
+        _logger.info("Segment: {} is consistent with latest table config and schema", segmentName);


This can flood the log (consider thousands of segments on each server)

removed the log

jackjlli · 2024-08-13T23:40:18Z

...mon/src/main/java/org/apache/pinot/common/restlet/resources/SegmentsReloadCheckResponse.java

+  private final String _serverInstanceId;
+
+  @JsonCreator
+  public SegmentsReloadCheckResponse(@JsonProperty("isReload") boolean isReload,


nit: needReload instead?

jackjlli · 2024-08-13T23:46:58Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

+      @Context HttpHeaders headers) {
+    tableName = DatabaseUtils.translateTableName(tableName, headers);
+    TableDataManager tableDataManager = ServerResourceUtils.checkGetTableDataManager(_serverInstance, tableName);
+    boolean isSegmentsReload = tableDataManager.needReloadSegments();


nit: rename the output to sth like needReload?

jackjlli · 2024-08-13T23:48:24Z

...mon/src/main/java/org/apache/pinot/common/restlet/resources/SegmentsReloadCheckResponse.java

+
+  @JsonCreator
+  public SegmentsReloadCheckResponse(@JsonProperty("isReload") boolean isReload,
+      @JsonProperty("serverInstanceId") String serverInstanceId) {


I wonder why the serverInstanceId needs to be returned as part of the response payload here? Didn't we already know where the request was sent to?

jackjlli · 2024-08-13T23:54:15Z

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

+      //if re processing or reload is needed on a segment then return true
+      return ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig, schema);
+    } catch (Exception e) {
+      _logger.error("Failed to check if reload is needed for a segment {}", segmentName, e);


Can we emit a server metric to indicate that something is wrong when running the check?

I feel that might be overkilling. IMO we can simply throw the exception out, and client can handle it accordingly.

jackjlli · 2024-08-13T23:57:18Z

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

+  public boolean needReloadSegments() {
+    IndexLoadingConfig indexLoadingConfig = fetchIndexLoadingConfig();
+    List<SegmentDataManager> segmentDataManagers = acquireAllSegments();
+    boolean segmentsReloadCheck = false;


nit: rename it to sth like needSegmentReload

jackjlli · 2024-08-13T23:59:55Z

...egment-local/src/main/java/org/apache/pinot/segment/local/data/manager/TableDataManager.java

+   * Checks for a particular segment, reload is needed or not
+   * @return true if the reload is needed on the segment
+   */
+  boolean checkReloadSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig indexLoadingConfig);


nit: unify the method name to sth like needReloadSegment(...)?

I don't think we need this API anymore

jackjlli · 2024-08-14T00:02:03Z

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

+    } catch (Exception e) {
+      _logger.error("Failed to check if reload is needed for a segment {}", segmentName, e);
+      closeSegmentDirectoryQuietly(segmentDirectory);
+      return false;


I saw you stated in the API description that 500 will be returned if there is any server side error. Shouldn't we return 500 here in this case?

remove unused imports

Remove serverInstanceId from the response

Fix the test

Jackie-Jiang · 2024-08-16T00:18:16Z

...mon/src/main/java/org/apache/pinot/common/restlet/resources/SegmentsReloadCheckResponse.java

+    return _serverInstanceId;
+  }
+
+  public boolean getReload() {


This method name might not work for JSON serialization. I think it will be mistakenly serialized as reload. Can you add a test to verify? We might need to rename it to isNeedReload(), or explicitly annotate it with @JsonProperty("needReload"). If we explicitly annotate, a better name could be isReloadNeeded()

Jackie-Jiang · 2024-08-16T00:19:49Z

...mon/src/main/java/org/apache/pinot/common/restlet/resources/SegmentsReloadCheckResponse.java

+ */
+public class SegmentsReloadCheckResponse {
+  private final boolean _needReload;
+  private final String _serverInstanceId;


(minor) instanceId might be more concise because this is always returned from server.

Do we really need to return it? Controller should already have this info when reading the address of the server

We have it, it's just little complex to separate out server instance when doing multi get request which is why just returning it as well in the response to make it simpler. I will look into this later. For now, we can include instanceId as well I think

Jackie-Jiang · 2024-08-16T00:44:34Z

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

+      throws Exception {
+    String segmentName = zkMetadata.getSegmentName();
+    SegmentDirectory segmentDirectory =
+        tryInitSegmentDirectory(segmentName, String.valueOf(zkMetadata.getCrc()), indexLoadingConfig);


This can cause exception for consuming segments. We should only check immutable segments. We can read the SegmentDirectory out from ImmutableSegmentImpl. We can consider adding a test in BaseClusterIntegrationTestSet.testReload() to verify if the API works. You may also add the test after the controller side integration.

Checked for ImmutableSegmentImpl instance before passing it through needReload where SegmentDirectory is generated. I think this should avoid any exception cases of consuming segments. Adding more tests along with controller side code and to combine with the same PR.

Jackie-Jiang · 2024-08-16T00:45:00Z

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

+    boolean needReload = false;
+    try {
+      for (SegmentDataManager segmentDataManager : segmentDataManagers) {
+        SegmentZKMetadata segmentZKMetadata = fetchZKMetadata(segmentDataManager.getSegmentName());


Let's not read ZK metadata because it needs to access ZK, and we might end up reading it for thousands of segments.

Jackie-Jiang · 2024-08-16T00:45:32Z

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

@@ -1013,6 +1013,22 @@ public boolean tryLoadExistingSegment(SegmentZKMetadata zkMetadata, IndexLoading
    }
  }

+  boolean needReloadSegment(SegmentZKMetadata zkMetadata, IndexLoadingConfig indexLoadingConfig)


Make it private

Jackie-Jiang · 2024-08-16T00:46:18Z

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

+      Schema schema = indexLoadingConfig.getSchema();
+      //if re processing or reload is needed on a segment then return true
+      return ImmutableSegmentLoader.needPreprocess(segmentDirectory, indexLoadingConfig, schema);
+    } catch (Exception e) {


We can probably directly throw the exception out without catch

closeSegmentDirectoryQuietly(segmentDirectory); There is this closure being handled in exception cases( so wrote a catch block). Do you want me to handle this in finally?

If we can use the SegmentDirectory within ImmutableSegmentImpl, we don't need to close it here. Ideally we don't want to create new SegmentDirectory in this check

Add Controller Side API and test in integration

Refactoring changes and test failures fixes

reverse merge

Code Readability and serializer tests

Jackie-Jiang

Mostly good

...c/main/java/org/apache/pinot/common/restlet/resources/ServerSegmentsReloadCheckResponse.java

...rc/main/java/org/apache/pinot/common/restlet/resources/TableSegmentsReloadCheckResponse.java

pinot-common/src/test/java/org/apache/pinot/common/utils/SerializerResponseTest.java

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

Jackie-Jiang · 2024-08-26T20:06:24Z

pinot-controller/src/main/java/org/apache/pinot/controller/util/TableMetadataReader.java

+  public Map<String, JsonNode> getServerCheckSegmentsReloadMetadata(String tableName, TableType tableType,
+      int timeoutMs)
+      throws InvalidConfigException, IOException {
+    String tableNameWithType =


This check should be performed in the RestletResource class before calling this method

Jackie-Jiang · 2024-08-26T20:07:29Z

pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java

@@ -721,6 +721,11 @@ public String reloadOfflineTable(String tableName, boolean forceDownload)
    return getControllerRequestClient().reloadTable(tableName, TableType.OFFLINE, forceDownload);
  }

+  public String needReloadOfflineTable(String tableNameWithType)


Rename it to checkIfReloadIsNeeded. Does it only apply to offline table?

It applies to both realtime and offline tables, changing the name

...rc/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java

Jackie-Jiang · 2024-08-26T20:16:35Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java

@@ -239,6 +239,11 @@ public String forTableReload(String tableName, TableType tableType, boolean forc
    return StringUtil.join("/", _baseUrl, "segments", tableName, query);
  }

+  public String forTableNeedReload(String tableNameWithType) {
+    String query = String.format("needReload?verbose=%s", "false");


(minor) No need to format

Tested for false and true verbose flags. Changed this method

Jackie-Jiang

LGTM

pinot-controller/src/main/java/org/apache/pinot/controller/util/TableMetadataReader.java

...rc/main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentImpl.java

pinot-spi/src/main/java/org/apache/pinot/spi/utils/builder/ControllerRequestURLBuilder.java

Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>

…hi912/pinot into Reload_Segment_Check_12117

…s reload of a table in servers (apache#13789)

deepthi912 added 2 commits August 9, 2024 14:10

apache#12117 Support for Server API to check for Segments reload

03ab3be

apache#12117

checkstyle exception

557eb73

checkstyle exception

deepthi912 changed the title ~~#12117 Support for Server API to check for Segments reloaded for a particular table~~ #12117 Support for Server API to check for Segments reload for a particular table in a server Aug 9, 2024

line characters checkstyle

ab901a3

line characters checkstyle

deepthi912 changed the title ~~#12117 Support for Server API to check for Segments reload for a particular table in a server~~ #12117 Support for Server API to check for Segments reload of a table in a server Aug 9, 2024

npawar reviewed Aug 12, 2024

View reviewed changes

deepthi912 added 3 commits August 12, 2024 16:47

segments reload check including indexes metadata

4e4a961

segments reload check including indexes metadata

checkstyle unused import removal

e3ea6a8

checkstyle unused import removal

remove unused code to get the columns

de79260

remove unused code to get the columns

siddharthteotia requested a review from jackjlli August 13, 2024 16:01

Jackie-Jiang added documentation observability rest-api labels Aug 13, 2024

Jackie-Jiang reviewed Aug 13, 2024

View reviewed changes

deepthi912 added 4 commits August 13, 2024 15:19

refactoring and review comments fixes

36beb11

checkstyle changes for the condition

0ee036f

checkstyle exception for import

c8ba979

change the flag name

e1c7c64

jackjlli reviewed Aug 14, 2024

View reviewed changes

deepthi912 added 8 commits August 13, 2024 17:55

review comments on variable names and response

b9e4fbf

test to check for reload segment when adding index

eef4de5

Remove unimplemented method and make private

9b904c6

Change error responses

38992b1

remove unused imports

d10b8b2

remove unused imports

Remove serverInstanceId from the response

12656ec

Remove serverInstanceId from the response

Fix the test

b987f0e

Fix the test

Add server instance id back

c6aa9ca

Jackie-Jiang reviewed Aug 16, 2024

View reviewed changes

review comments fixes

8848fa6

Add Controller Side API and test in integration

5c50cca

Add Controller Side API and test in integration

deepthi912 changed the title ~~#12117 Support for Server API to check for Segments reload of a table in a server~~ #12117 Support for Server & Controller API to check for Segments reload of a table in servers Aug 16, 2024

deepthi912 and others added 12 commits August 16, 2024 17:12

Refactoring and checkstyle fixes

8366580

import issue

25ab04c

unused import removal

760450f

checkstyle fixes

ff0f1a1

checkstyle exception fix

a5880c2

Refactoring changes and test failures fixes

a0006c9

Refactoring changes and test failures fixes

Merge pull request #20 from apache/master

9bac29a

reverse merge

Code Readability and serializer tests

84bb84b

Code Readability and serializer tests

checkstyle fixes

5491662

checkstyle fixes

8173c1c

remove unused method

3649c78

add json properties

a14af52

Jackie-Jiang reviewed Aug 26, 2024

View reviewed changes

deepthi912 added 3 commits August 26, 2024 15:19

Review Comments Fixes

ad11da4

Naming fixes

468b36d

Naming changes

d1b330a

Jackie-Jiang approved these changes Aug 28, 2024

View reviewed changes

deepthi912 and others added 7 commits August 27, 2024 17:17

Update to Boolean variable

ff056b0

Co-authored-by: Xiaotian (Jackie) Jiang <17555551+Jackie-Jiang@users.noreply.github.com>

Changes in names

33a2408

Merge branch 'Reload_Segment_Check_12117' of https://github.com/deept…

3d7a0ee

…hi912/pinot into Reload_Segment_Check_12117

checkstyle fixes

5f555e7

change the format of the comment

efa1766

test case fix

00d2220

test case random failures

aa3a722

Jackie-Jiang merged commit ae97265 into apache:master Aug 28, 2024
21 checks passed

rajagopr pushed a commit to rajagopr/pinot that referenced this pull request Sep 11, 2024

apache#12117 Support for Server & Controller API to check for Segment…

2177c6a

…s reload of a table in servers (apache#13789)

Jackie-Jiang mentioned this pull request Nov 14, 2024

[Flaky-test] HybridClusterIntegrationTest.testReload() #14456

Closed

yashmayya mentioned this pull request Nov 15, 2024

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

Merged

#12117 Support for Server & Controller API to check for Segments reload of a table in servers #13789

#12117 Support for Server & Controller API to check for Segments reload of a table in servers #13789

Conversation

deepthi912 commented Aug 9, 2024 • edited Loading

codecov-commenter commented Aug 9, 2024 • edited Loading

Codecov Report

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

Jackie-Jiang left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a 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

Jackie-Jiang left a comment

Choose a reason for hiding this comment

deepthi912 commented Aug 9, 2024 •

edited

Loading

codecov-commenter commented Aug 9, 2024 •

edited

Loading