-
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
track newly added segments for upsert tables for a more complete upsert data view #13992
track newly added segments for upsert tables for a more complete upsert data view #13992
Conversation
2ee3554
to
42019ca
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13992 +/- ##
============================================
+ Coverage 61.75% 65.04% +3.28%
- Complexity 207 1534 +1327
============================================
Files 2436 2538 +102
Lines 133233 140268 +7035
Branches 20636 21562 +926
============================================
+ Hits 82274 91231 +8957
+ Misses 44911 42309 -2602
- Partials 6048 6728 +680
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
After some offline discussion, suggest making the following changes:
- Make the config something like
newSegmentTrackingTimeMs
because optional is internal detail but not user friendly - Remove clean-up interval, and cleanup synchronously when query comes in
- Decouple it from
UpsertViewManager
, and we can treat non-positive tracking time as disabling the feature
42019ca
to
e92ec54
Compare
…s upsert table even not using consistency mode
e92ec54
to
158a43b
Compare
.../src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java
Show resolved
Hide resolved
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.
LGTM with one non-blocking comment
aa04b64
to
f007783
Compare
f007783
to
d2eda3b
Compare
When uploading a segment, the server may process it ahead of broker adding it to routing table. Broker listens to IS changes to add the newly added segments into routing table, which may take a few seconds or many seconds, depending on the load of the broker.
If query gets routed by broker during this short window, broker may not include the new segment in the list of selected segments for the query, although the server already starts to process the new segment and invalidate the docs in existing segments. Due to this race condition, the query may access less than expected valid docs.
In this PR, we tracks newly uploaded segments on server side for a configurable period, so that server can ask query to access them as optional segments. Optional segment is a mechanism we added earlier on to handle a similar race condition for consuming segment.
This PR adds a new config
newSegmentTrackingTimeMs
to enable this feature, essentially as a best effort to wait for broker to add a segment into their routing table upon IS changes. By default, it's 10s to enable this tracking feature.