Minor Refactoring and fixes #15419

noob-se7en · 2025-03-31T19:02:34Z

Minor refactoring post #15404

noob-se7en · 2025-03-31T19:10:04Z

...re/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java

+        // before acquiring semaphore. These scenarios can lead to creation of small segments because segment
+        // consumption is terminated early.
+        _consumeStartTime = now();
+        setConsumeEndTime(_segmentZKMetadata, _consumeStartTime);


Not much benefit of this. Since mostly the endCriteriaTime is determined from zkMetadata only and extra consume time would be max 10 minutes in rare cases.
Probably need a revisit.

This has side effect of different replicas stops at different time. We need to use the time in the constructor so that the time is relatively aligned for the commit protocol to work

I see. Although this can happen currently as well where The consuming segment was just about to be completed, but that server went down.

codecov-commenter · 2025-03-31T19:48:03Z

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 62.90%. Comparing base (9145e92) to head (d8023c7).
Report is 22 commits behind head on master.

Files with missing lines	Patch %	Lines
...a/manager/realtime/RealtimeSegmentDataManager.java	85.71%	0 Missing and 1 partial ⚠️
...ata/manager/realtime/RealtimeTableDataManager.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #15419      +/-   ##
============================================
- Coverage     62.95%   62.90%   -0.05%     
- Complexity     1381     1383       +2     
============================================
  Files          2848     2863      +15     
  Lines        161547   162439     +892     
  Branches      24752    24879     +127     
============================================
+ Hits         101700   102182     +482     
- Misses        52167    52573     +406     
- Partials       7680     7684       +4

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`62.82% <75.00%> (-0.09%)`	⬇️
java-21	`62.87% <75.00%> (-0.07%)`	⬇️
skip-bytebuffers-false	`62.83% <75.00%> (-0.12%)`	⬇️
skip-bytebuffers-true	`62.86% <75.00%> (-0.04%)`	⬇️
temurin	`62.90% <75.00%> (-0.05%)`	⬇️
unittests	`62.90% <75.00%> (-0.05%)`	⬇️
unittests1	`55.86% <75.00%> (-0.06%)`	⬇️
unittests2	`33.55% <0.00%> (-0.13%)`	⬇️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Jackie-Jiang · 2025-04-10T17:05:37Z

...core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java

@@ -518,15 +518,13 @@ public void addConsumingSegment(String segmentName)

  private void doAddConsumingSegment(String segmentName)
      throws AttemptsExceededException, RetriableOperationException {
-    SegmentZKMetadata zkMetadata = fetchZKMetadata(segmentName);
-    if (!_enforceConsumptionInOrder && zkMetadata.getStatus().isCompleted()) {
+    SegmentZKMetadata zkMetadata = fetchZKMetadataNullable(segmentName);


We do want it to throw exception when ZK metadata is removed. This is an unexpected scenario

We do want it to throw exception when ZK metadata is removed.

That will put segment in error state.

This is an unexpected scenario

Why? Like this is valid scenario:

offline -> consuming

consuming -> dropped

We can't conclude actually if this is unexpected.

This is to be consistent with other state transition handling. When a segment is deleted, we throw exception during ZK metadata check, and then ERROR -> DROPPED state transition should be able to remove it properly

E.g. in doAddOnlineSegment() we don't allow null ZK metadata

got it. fixed this.

Jackie-Jiang · 2025-04-10T17:12:04Z

...re/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java

    } catch (Throwable t) {
      // In case of exception thrown here, segment goes to ERROR state. Then any attempt to reset the segment from
      // ERROR -> OFFLINE -> CONSUMING via Helix Admin fails because the semaphore is acquired, but not released.
      // Hence releasing the semaphore here to unblock reset operation via Helix Admin.
-      releaseConsumerSemaphore();


Jackie-Jiang · 2025-04-10T17:13:04Z

...re/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java

@@ -1722,16 +1726,13 @@ public RealtimeSegmentDataManager(SegmentZKMetadata segmentZKMetadata, TableConf
      _state = State.INITIAL_CONSUMING;
      _latestStreamOffsetAtStartupTime = fetchLatestStreamOffset(5000);
      _consumeStartTime = now();
-      setConsumeEndTime(segmentZKMetadata, _consumeStartTime);
+      setConsumeEndTime(segmentZKMetadata, now());


(minor) This can be reverted

Actually _consumeStartTime should not be initiliased in contructor.

Jackie-Jiang · 2025-04-10T17:14:22Z

...re/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java

@@ -747,6 +747,10 @@ public void run() {

        _segmentLogger.info("Acquired consumer semaphore.");

+        _consumeStartTime = now();
+        _segmentLogger.info("Starting consumption on realtime consuming segment {} maxRowCount {} maxEndTime {}",


(minor)

Suggested change

_segmentLogger.info("Starting consumption on realtime consuming segment {} maxRowCount {} maxEndTime {}",

_segmentLogger.info("Starting consumption on segment: {}, maxRowCount: {}, maxEndTime: {}",

Jackie-Jiang · 2025-04-10T17:16:14Z

...re/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java

@@ -311,7 +311,7 @@ public void deleteSegmentFile() {
  private final AtomicLong _lastUpdatedRowsIndexed = new AtomicLong(0);
  private final String _instanceId;
  private final ServerSegmentCompletionProtocolHandler _protocolHandler;
-  private final long _consumeStartTime;
+  private long _consumeStartTime;


(minor) Move it in front of _lastLogTime

noob-se7en · 2025-04-10T21:05:57Z

...rc/test/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManagerTest.java

-        if (_timeCheckCounter.incrementAndGet() <= FakeStreamConfigUtils.SEGMENT_FLUSH_THRESHOLD_ROWS + 4) {
+        // now() is called once in the run() method, then once on setting consumeStartTime, once before each batch
+        // reading and once for every row indexation
+        if (_timeCheckCounter.incrementAndGet() <= FakeStreamConfigUtils.SEGMENT_FLUSH_THRESHOLD_ROWS + 5) {


tried removing this painful test depending on _timeCheckCounter but the usage of it is spread across and requires some good refactoring and was taking longer than expected

noob-se7en added 4 commits April 1, 2025 00:10

Refresh segment consumption end time

f7f9504

nit

7edea7a

minor fixes

a18c85e

nit

5242490

noob-se7en commented Mar 31, 2025

View reviewed changes

noob-se7en changed the title ~~Refresh segment consumption end time~~ Minor Refactoring Mar 31, 2025

noob-se7en added 3 commits April 10, 2025 21:44

Addresses PR comment

1feadbf

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

6781cfd

fix condition

42037b7

noob-se7en requested a review from Jackie-Jiang April 10, 2025 16:36

noob-se7en changed the title ~~Minor Refactoring~~ Minor Refactoring and fixes Apr 10, 2025

noob-se7en changed the title ~~Minor Refactoring and fixes~~ Minor Refactoring and fixes in Ingestion Apr 10, 2025

Jackie-Jiang reviewed Apr 10, 2025

View reviewed changes

Jackie-Jiang added ingestion bugfix real-time labels Apr 10, 2025

noob-se7en added 2 commits April 11, 2025 00:55

handle consumeStartTime for online segment

e9d031d

fixes painful test

2b06969

noob-se7en commented Apr 10, 2025

View reviewed changes

fixes lint

351b4fb

noob-se7en requested a review from Jackie-Jiang April 10, 2025 21:08

noob-se7en changed the title ~~Minor Refactoring and fixes in Ingestion~~ Minor Refactoring and fixes Apr 11, 2025

revert zkmetadata nullable check

d8023c7

Jackie-Jiang approved these changes Apr 15, 2025

View reviewed changes

Jackie-Jiang merged commit 1fe72ae into apache:master Apr 15, 2025
21 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor Refactoring and fixes #15419

Minor Refactoring and fixes #15419

noob-se7en commented Mar 31, 2025

noob-se7en Mar 31, 2025 •

edited

Loading

Jackie-Jiang Apr 2, 2025

noob-se7en Apr 10, 2025

noob-se7en Apr 10, 2025

codecov-commenter commented Mar 31, 2025 •

edited

Loading

Jackie-Jiang Apr 10, 2025

noob-se7en Apr 10, 2025 •

edited

Loading

noob-se7en Apr 10, 2025

Jackie-Jiang Apr 14, 2025

Jackie-Jiang Apr 14, 2025

noob-se7en Apr 15, 2025

Jackie-Jiang Apr 10, 2025

Jackie-Jiang Apr 10, 2025

noob-se7en Apr 10, 2025

Jackie-Jiang Apr 10, 2025

Jackie-Jiang Apr 10, 2025

noob-se7en Apr 10, 2025 •

edited

Loading

	_segmentLogger.info("Starting consumption on realtime consuming segment {} maxRowCount {} maxEndTime {}",
	_segmentLogger.info("Starting consumption on segment: {}, maxRowCount: {}, maxEndTime: {}",

Minor Refactoring and fixes #15419

Minor Refactoring and fixes #15419

Conversation

noob-se7en commented Mar 31, 2025

noob-se7en Mar 31, 2025 • 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

codecov-commenter commented Mar 31, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

noob-se7en Apr 10, 2025 • 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

noob-se7en Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

noob-se7en Mar 31, 2025 •

edited

Loading

codecov-commenter commented Mar 31, 2025 •

edited

Loading

noob-se7en Apr 10, 2025 •

edited

Loading

noob-se7en Apr 10, 2025 •

edited

Loading