Fix issue with realtime partition mismatch metric #11871

sajjad-moradi · 2023-10-25T19:36:33Z

Fixes the issue described in #11870

sajjad-moradi · 2023-10-25T19:39:13Z

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

@@ -1577,7 +1577,6 @@ private void setPartitionParameters(RealtimeSegmentConfig.Builder realtimeSegmen
            _segmentLogger.warn(
                "Number of stream partitions: {} does not match number of partitions in the partition config: {}, "
                    + "using number of stream " + "partitions", numPartitionGroups, numPartitions);
-            _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.REALTIME_PARTITION_MISMATCH, 1);


Emitting the metric here is wrong, because numPartitions fetched form the stream is used if there's a mismatch between table config and stream metadata. So in reality no event with incorrect partition is consumed here.

Also, we should change the logger to an info log.

Better yet, drop the comparison altogether between the tableconfig value and the number of partitions we come across in the stream. Two reasons:

The tableconfig value is not used while routing queries to realtime tables

Stream partitions change dynamically, and we don't update the table config when we detect new partitions. So, it is impossible to keep them in sync.

I believe the best action item here is to drop numPartition from table config altogether as it shouldn't be used anywhere, and does not provide any value.
To keep the scope of this PR limited to the metric emission fix, I'm just changing the log level to info.

numPartitions in the table config seems to be used for instance assignment when a table is created (or rebalanced). I am not able to determine easily how the actual number of partitions in the topic is used for instance creation instead of the configured value (for realtime tables). The value is also used for table rebalancing.

Lastly, realtime and offline tables have the same table config definition.

So we cannot really remove the num partitions from table config definitions.

But it is clear that there is no use for this field for realtime tables, and hence there is really no need to compare this with the actual number of partitions, and hence the suggestion to remove the compare.

@Jackie-Jiang what do you think?

I am also ok to keep it as an info, but I feel it is extraneous code. If we can remove it, we should.

Taking a closer look at the logic and I feel we are making wrong assumptions based on Kafka behavior. numPartitions is used to let pinot know how the data is supposed to be partitioned. For Kafka, data is always partitioned by the upstream Kafka partition, but this assumption doesn't hold for all streams. The clean solution is to introduce an interface in the stream metadata provider to provide the actual numPartitions to use by passing in the value from the table config.

@Jackie-Jiang are you ok with creating an issue to clean this up, because we are facing this problem in production now.

codecov-commenter · 2023-10-25T20:21:54Z

Codecov Report

Merging #11871 (db11c9c) into master (649477a) will increase coverage by 26.58%.
Report is 1 commits behind head on master.
The diff coverage is 37.50%.

@@              Coverage Diff              @@
##             master   #11871       +/-   ##
=============================================
+ Coverage     34.84%   61.43%   +26.58%     
- Complexity      946     1147      +201     
=============================================
  Files          2297     2373       +76     
  Lines        124532   128277     +3745     
  Branches      19245    19804      +559     
=============================================
+ Hits          43395    78808    +35413     
+ Misses        78111    43771    -34340     
- Partials       3026     5698     +2672

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (ø)`
integration	`<0.01% <0.00%> (ø)`
integration1	`<0.01% <0.00%> (ø)`
integration2	`0.00% <0.00%> (ø)`
java-11	`61.33% <37.50%> (+14.70%)`	⬆️
java-21	`61.31% <37.50%> (+26.59%)`	⬆️
skip-bytebuffers-false	`61.42% <37.50%> (+26.58%)`	⬆️
skip-bytebuffers-true	`61.23% <37.50%> (+26.54%)`	⬆️
temurin	`61.43% <37.50%> (+26.58%)`	⬆️
unittests	`61.43% <37.50%> (+14.76%)`	⬆️
unittests1	`46.64% <0.00%> (-0.03%)`	⬇️
unittests2	`27.61% <37.50%> (?)`

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

Files	Coverage Δ
...a/manager/realtime/RealtimeSegmentDataManager.java	`50.66% <0.00%> (+0.06%)`	⬆️
...local/indexsegment/mutable/MutableSegmentImpl.java	`71.84% <42.85%> (+71.84%)`	⬆️

... and 825 files with indirect coverage changes

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

mcvsubbu · 2023-10-25T20:59:11Z

...al/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java

@@ -138,6 +138,7 @@ public class MutableSegmentImpl implements MutableSegment {
  private final RealtimeSegmentStatsHistory _statsHistory;
  private final String _partitionColumn;
  private final PartitionFunction _partitionFunction;
+  private final int _mainPartitionId;


Suggested change

private final int _mainPartitionId;

private final int _consumingPartitionId;

Just a suggestion. Or else, add a comment there as to what you mean by "main"

mcvsubbu · 2023-10-25T20:59:52Z

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

@@ -1577,7 +1577,6 @@ private void setPartitionParameters(RealtimeSegmentConfig.Builder realtimeSegmen
            _segmentLogger.warn(
                "Number of stream partitions: {} does not match number of partitions in the partition config: {}, "
                    + "using number of stream " + "partitions", numPartitionGroups, numPartitions);
-            _serverMetrics.addMeteredTableValue(_tableNameWithType, ServerMeter.REALTIME_PARTITION_MISMATCH, 1);


Also, we should change the logger to an info log.

Better yet, drop the comparison altogether between the tableconfig value and the number of partitions we come across in the stream. Two reasons:

The tableconfig value is not used while routing queries to realtime tables

Stream partitions change dynamically, and we don't update the table config when we detect new partitions. So, it is impossible to keep them in sync.

mcvsubbu · 2023-10-26T17:28:34Z

A minor ask: Can you add a comment in the line where the metric REALTIME_PARTITION_MISMATCH is defined that it is a count of the number of records that are mismatched? It may help prevent someone from adding it back to this if statement.
Thanks, otherwise I am fine with this PR.

mcvsubbu

In the interest of time, I am approving this. Please do the following:

Create an issue (or keep the current one open) with Jackie's suggestion in it.
Add a comment in ServerMetrics to reflect the meaning of REALTIME_PARTITION_MISMATCH metric.

Thanks!

Fix issue with realtime partition mismatch metric

f7c91a9

sajjad-moradi requested review from mcvsubbu and Jackie-Jiang October 25, 2023 19:36

sajjad-moradi commented Oct 25, 2023

View reviewed changes

mcvsubbu reviewed Oct 25, 2023

View reviewed changes

Address comments

db11c9c

mcvsubbu approved these changes Oct 26, 2023

View reviewed changes

Add a comment on the metric definition

bfb1111

sajjad-moradi merged commit 8feec41 into apache:master Oct 27, 2023
16 of 19 checks passed

sajjad-moradi deleted the bug/fix.realtime.partition.mismatch.metric branch October 27, 2023 01:21

sajjad-moradi mentioned this pull request Oct 27, 2023

REALTIME_PARTITION_MISMATCH metric is emitted only once #11870

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with realtime partition mismatch metric #11871

Fix issue with realtime partition mismatch metric #11871

sajjad-moradi commented Oct 25, 2023

sajjad-moradi Oct 25, 2023 •

edited

Loading

mcvsubbu Oct 25, 2023

sajjad-moradi Oct 26, 2023 •

edited

Loading

mcvsubbu Oct 26, 2023

Jackie-Jiang Oct 26, 2023

mcvsubbu Oct 26, 2023

codecov-commenter commented Oct 25, 2023 •

edited

Loading

mcvsubbu Oct 25, 2023

mcvsubbu Oct 25, 2023

mcvsubbu commented Oct 26, 2023

mcvsubbu left a comment

	private final int _mainPartitionId;
	private final int _consumingPartitionId;

Fix issue with realtime partition mismatch metric #11871

Fix issue with realtime partition mismatch metric #11871

Conversation

sajjad-moradi commented Oct 25, 2023

sajjad-moradi Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

mcvsubbu Oct 25, 2023

Choose a reason for hiding this comment

sajjad-moradi Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

mcvsubbu Oct 26, 2023

Choose a reason for hiding this comment

Jackie-Jiang Oct 26, 2023

Choose a reason for hiding this comment

mcvsubbu Oct 26, 2023

Choose a reason for hiding this comment

codecov-commenter commented Oct 25, 2023 • edited Loading

Codecov Report

mcvsubbu Oct 25, 2023

Choose a reason for hiding this comment

mcvsubbu Oct 25, 2023

Choose a reason for hiding this comment

mcvsubbu commented Oct 26, 2023

mcvsubbu left a comment

Choose a reason for hiding this comment

sajjad-moradi Oct 25, 2023 •

edited

Loading

sajjad-moradi Oct 26, 2023 •

edited

Loading

codecov-commenter commented Oct 25, 2023 •

edited

Loading