Updates forceCommit APIs to handle Pauseless #14828

noob-se7en · 2025-01-16T12:37:01Z

Updates forceCommit APIs to handle Pauseless Ingestion.

(In Pauseless Ingestion, The segment is marked as ONLINE in the ideal state before the commit has completed successfully. Hence the check needs to be updated from ideal state to ZK metadata which is the correct indicator)

codecov-commenter · 2025-01-16T13:17:41Z

Codecov Report

Attention: Patch coverage is 45.45455% with 12 lines in your changes missing coverage. Please review.

Project coverage is 63.75%. Comparing base (59551e4) to head (9e3ddad).
Report is 1617 commits behind head on master.

Files with missing lines	Patch %	Lines
...ller/api/resources/PinotRealtimeTableResource.java	0.00%	12 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14828      +/-   ##
============================================
+ Coverage     61.75%   63.75%   +2.00%     
- Complexity      207     1469    +1262     
============================================
  Files          2436     2708     +272     
  Lines        133233   151438   +18205     
  Branches      20636    23381    +2745     
============================================
+ Hits          82274    96549   +14275     
- Misses        44911    47645    +2734     
- Partials       6048     7244    +1196

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.72% <45.45%> (+2.02%)`	⬆️
java-21	`63.62% <45.45%> (+1.99%)`	⬆️
skip-bytebuffers-false	`63.74% <45.45%> (+2.00%)`	⬆️
skip-bytebuffers-true	`63.60% <45.45%> (+35.87%)`	⬆️
temurin	`63.75% <45.45%> (+2.00%)`	⬆️
unittests	`63.75% <45.45%> (+2.00%)`	⬆️
unittests1	`56.33% <ø> (+9.44%)`	⬆️
unittests2	`34.03% <45.45%> (+6.30%)`	⬆️

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.

Jackie-Jiang

Mostly good. Good job!

Jackie-Jiang · 2025-01-17T03:15:49Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

+    jobMetadata.put(CommonConstants.ControllerJob.CONSUMING_SEGMENTS_YET_TO_BE_COMMITTED_LIST,
+        JsonUtils.objectToString(consumingSegmentsCommitted));


(minor) This shouldn't be needed. Adding it will actually add overhead for one extra parsing

Jackie-Jiang · 2025-01-17T03:21:43Z

...ain/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java

+      if (segmentZKMetadata == null) {
+        continue;
+      }
+      if (!segmentZKMetadata.getStatus().isCompleted()) {


We cannot use isCompleted() here. We should explicitly check for it to be DONE

@9aman @KKcorps Currently COMMITTING is also count as completed, is this expected?

isCompleted should return true only when the stratus is Uploaded or Done. Raising a PR along with few minor improvement wrt pauseless,

Jackie-Jiang · 2025-01-17T03:23:09Z

...ain/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java

+      SegmentZKMetadata segmentZKMetadata =
+          _helixResourceManager.getSegmentZKMetadata(tableNameWithType, segmentName);
+      if (segmentZKMetadata == null) {
+        continue;


(minor) Add some comment about this behavior. We are counting deleted segment as not need to be committed

Jackie-Jiang · 2025-01-17T03:25:04Z

...ntroller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java

+      Map<String, String> controllerJobZKMetadata) {
+    addControllerJobToZK(forceCommitJobId,
+        controllerJobZKMetadata, ControllerJobType.FORCE_COMMIT, prevJobMetadata -> {
+          String existingSegmentsYetToBeCommittedString =


Please add some comments describing why we want to perform this check

Actually this method is only useful for a rare edge case when two async forceCommitStatus APIs are running.
But we can remove this as it will save overhead for one extra parsing on each API call.

have removed this method

KKcorps · 2025-01-17T10:38:07Z

...ests/src/test/java/org/apache/pinot/integration/tests/LLCRealtimeClusterIntegrationTest.java

@@ -462,6 +467,19 @@ public boolean isForceCommitJobCompleted(String forceCommitJobId)

    assertEquals(jobStatus.get("jobId").asText(), forceCommitJobId);
    assertEquals(jobStatus.get("jobType").asText(), "FORCE_COMMIT");
+
+    assert jobStatus.get("segmentsForceCommitted") != null;


use CommonConstants variable rather than hardcode

KKcorps

LGTM with minor comments

KKcorps · 2025-01-17T10:50:21Z

...java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManagerTest.java

+    when(mockSegmentZKMetadataUploaded.getStatus()).thenReturn(Status.UPLOADED);
+
+    SegmentZKMetadata mockSegmentZKMetadataInProgress = mock(SegmentZKMetadata.class);
+    when(mockSegmentZKMetadataInProgress.getStatus()).thenReturn(Status.IN_PROGRESS);


also add one for Status.COMMITTING

…mit_status

9aman · 2025-01-23T07:48:04Z

...ller/src/main/java/org/apache/pinot/controller/api/resources/PinotRealtimeTableResource.java

+      segmentsToCheck = consumingSegmentCommitted;
+    }
+
+    Set<String> segmentsYetToBeCommitted =


@noob-se7en iiuc the objective of introducing this field is to reduce the number of segmentZKMetadata that we will be iterating over.

Does it server any other purpose ?

Yes, Just to reduce the ZK calls only.

9aman

LGTM

…mit_status

noob-se7en added 4 commits January 16, 2025 17:04

Updates foceCommit API to handle Pauseless

ab2220f

updates metadata

ed90f11

fixes lint

e88aa2a

adds tests

8c8d8d3

noob-se7en marked this pull request as ready for review January 16, 2025 12:40

saves 1 zk call

1be0316

noob-se7en mentioned this pull request Jan 16, 2025

Supports Force Committing Segments in Batches #14811

Open

Jackie-Jiang reviewed Jan 17, 2025

View reviewed changes

noob-se7en added 2 commits January 17, 2025 15:33

Addresses PR comments

095acc0

nit

36360b8

noob-se7en requested a review from Jackie-Jiang January 17, 2025 10:24

adds unit test

68cdc26

KKcorps reviewed Jan 17, 2025

View reviewed changes

noob-se7en added 3 commits January 17, 2025 16:32

Merge branch 'master' of github.com:apache/pinot into update_forceCom…

b8a2e7f

…mit_status

addresses PR comments

5730a06

nit

c8565d6

noob-se7en requested a review from KKcorps January 17, 2025 11:09

9aman reviewed Jan 23, 2025

View reviewed changes

noob-se7en added 4 commits January 23, 2025 15:59

attempt to fix test

857dd6a

Merge branch 'master' of github.com:apache/pinot into update_forceCom…

2f7e5d9

…mit_status

nit

0b64439

Attempts to fix test

9e3ddad

Jackie-Jiang added enhancement rest-api labels Jan 23, 2025

Jackie-Jiang approved these changes Jan 24, 2025

View reviewed changes

Jackie-Jiang merged commit c9e08f3 into apache:master Jan 24, 2025
21 checks passed

gortiz pushed a commit to gortiz/pinot that referenced this pull request Jan 30, 2025

Updates forceCommit APIs to handle Pauseless (apache#14828)

a6d2208

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates forceCommit APIs to handle Pauseless #14828

Updates forceCommit APIs to handle Pauseless #14828

noob-se7en commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 16, 2025 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Jan 17, 2025

Jackie-Jiang Jan 17, 2025

9aman Jan 22, 2025

Jackie-Jiang Jan 17, 2025

Jackie-Jiang Jan 17, 2025

noob-se7en Jan 17, 2025

noob-se7en Jan 17, 2025

KKcorps Jan 17, 2025

KKcorps left a comment

KKcorps Jan 17, 2025

9aman Jan 23, 2025 •

edited

Loading

9aman Jan 23, 2025

noob-se7en Jan 23, 2025

9aman left a comment •

edited

Loading

		jobMetadata.put(CommonConstants.ControllerJob.CONSUMING_SEGMENTS_YET_TO_BE_COMMITTED_LIST,
		JsonUtils.objectToString(consumingSegmentsCommitted));

Updates forceCommit APIs to handle Pauseless #14828

Updates forceCommit APIs to handle Pauseless #14828

Conversation

noob-se7en commented Jan 16, 2025 • edited Loading

codecov-commenter commented Jan 16, 2025 • edited Loading

Codecov Report

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

KKcorps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9aman Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9aman left a comment • edited Loading

Choose a reason for hiding this comment

noob-se7en commented Jan 16, 2025 •

edited

Loading

codecov-commenter commented Jan 16, 2025 •

edited

Loading

9aman Jan 23, 2025 •

edited

Loading

9aman left a comment •

edited

Loading