tibrewalpratik17 commented on code in PR #12976:
URL: https://github.com/apache/pinot/pull/12976#discussion_r1575309595
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1103,6 +1210,48 @@ private static MutableRoaringBitmap
getQueryableDocIdsSnapshotFromSegment(IndexS
return queryableDocIdsSnapshot;
}
+ private void setSegmentContexts(List<SegmentContext> segmentContexts) {
+ for (SegmentContext segmentContext : segmentContexts) {
+ IndexSegment segment = segmentContext.getIndexSegment();
+ if (_trackedSegments.contains(segment)) {
+
segmentContext.setQueryableDocIdsSnapshot(getQueryableDocIdsSnapshotFromSegment(segment));
+ }
+ }
+ }
+
+ private boolean skipUpsertViewRefresh(long upsertViewFreshnessMs) {
+ long nowMs = System.currentTimeMillis();
+ if (upsertViewFreshnessMs >= 0) {
+ return _lastUpsertViewRefreshTimeMs + upsertViewFreshnessMs > nowMs;
+ }
+ return _lastUpsertViewRefreshTimeMs + _upsertViewRefreshIntervalMs > nowMs;
+ }
+
+ private void doBatchRefreshUpsertView(long upsertViewFreshnessMs) {
Review Comment:
Instead of doing this at partition metadata manager level can we move this
to table data manager level? We can maintain a map of segment -> segmentContext
and do a refresh there. This way we can easily extend the scope of segment
context in future as well.
I was trying on something like this --
https://github.com/apache/pinot/compare/master...tibrewalpratik17:pinot:fix_upsert_inconsistency?expand=1
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/UpsertConfig.java:
##########
@@ -75,6 +75,15 @@ public enum Strategy {
@JsonPropertyDescription("Whether to preload segments for fast upsert
metadata recovery")
private boolean _enablePreload;
+ @JsonPropertyDescription("Whether to enable consistent view for upsert
table")
+ private boolean _enableUpsertView;
+
+ @JsonPropertyDescription("Whether to enable batch refresh mode to keep
consistent view for upsert table")
+ private boolean _enableUpsertViewBatchRefresh;
Review Comment:
imo this should be enabled by default. At present, this is more of a bugfix
rather than a feature to be kept behind a config. It can be kept behind a
config initially to be on the safer side but then we should enable it by
default in the long run.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
