-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug fix: reset primary key count to 0 when table is deleted #12169
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #12169 +/- ##
============================================
- Coverage 61.63% 61.61% -0.02%
Complexity 1152 1152
============================================
Files 2407 2407
Lines 130888 130915 +27
Branches 20220 20225 +5
============================================
- Hits 80670 80662 -8
- Misses 44336 44367 +31
- Partials 5882 5886 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT, | ||
0L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do this during stop() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we want to finish all operations then reset the gauge, so here should be the right place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding some comments explaining why we reset the gauge here
_serverMetrics.setValueOfPartitionGauge(_tableNameWithType, _partitionId, ServerGauge.UPSERT_PRIMARY_KEYS_COUNT, | ||
0L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding some comments explaining why we reset the gauge here
1fcdc23
to
89e4c5c
Compare
We made change in the Table deletion flow to not remove segments from the upsert metadata manager and directly close it to save time.
However, due to this change, we run into a metric mismatch issue, where the primary key count still shows up as the last updated one instead of 0 or NULL in dashboards.
The fix is to set this metric to 0 explicitly when the metadata manager is closed.
This can be verified in the following way:
Download prometheus javagent https://repo.maven.apache.org/maven2/io/prometheus/jmx/jmx_prometheus_javaagent/0.20.0/jmx_prometheus_javaagent-0.20.0.jar
Run UpsertQuickstart with the following VM options
-ea -javaagent:jmx_prometheus_javaagent-0.20.0.jar=9021:$PINOT_GIT_DIR/docker/images/pinot/etc/jmx_prometheus_javaagent/configs/server.yml
Run jconsole and connect to quickstart VM
Check the metric
"org.apache.pinot.common.metrics":type="ServerMetrics",name="pinot.server.upsertPrimaryKeysCount.upsertMeetupRsvp_REALTIME.0"
Delete the table and check the metric again.