You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@pinot.apache.org by "klsince (via GitHub)" <gi...@apache.org> on 2024/04/19 21:27:22 UTC

[PR] [WIP] add locking logic to get consistent table view for upsert tables [pinot]

klsince opened a new pull request, #12976:
URL: https://github.com/apache/pinot/pull/12976

This PR tries to support consistent table view for querying upsert tables. The consistency is at table partition level, which can contain many segments. Today, updates that involve two segments' bitmaps are not atomic, thus causing queries to see inconsistent table view, e.g. to return less than expected PKs.

The high level idea is either to synchronize the query threads and upsert threads (including consuming thread or HelixTaskExecutor threads) for queries to get a consistent set of segments' validDocIds bitmaps; or let upsert threads keep a copy of bitmaps and refresh it regularly. Both modes are added in this PR, as they can be wired up using one R/W lock.

Configs:
1. by default, the feature is disabled.
2. if _enableUpsertView=true, the upsert threads take the WLock when the upsert involves two segments' bitmaps; and the query threads take the RLock when getting bitmaps for all its selected segments. This is the sync mode, for best data freshness, but queries may block data ingestion. Mainly for low qps or low ingestion cases.
3. if _enableUpsertViewBatchRefresh=true, the query threads don't need to take lock when getting bitmaps. The query threads access a copy of bitmaps that are kept updated by upsert thread periodically. In this batch mode, if data freshness is concerned, the query can specify a query option as called upsertViewFreshnessMs (e.g. 3s) to set its tolerance on data freshness, and the query thread can fresh the bitmap copies immediately if they are not fresh enough. For high qps and high ingestion cases.

--
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

Re: [PR] Add locking logic to get consistent table view for upsert tables [pinot]

Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.

Jackie-Jiang commented on code in PR #12976:
URL: https://github.com/apache/pinot/pull/12976#discussion_r1609034975


##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -1058,11 +1154,37 @@ protected void removeDocId(IndexSegment segment, int docId) {
    * Use the segmentContexts to collect the contexts for selected segments. Reuse the segmentContext object if
    * present, to avoid overwriting the contexts specified at the others places.
    */
-  public void setSegmentContexts(List<SegmentContext> segmentContexts) {
+  public void setSegmentContexts(List<SegmentContext> segmentContexts, Map<String, String> queryOptions) {
+    if (_consistencyMode == UpsertConfig.ConsistencyMode.NONE) {
+      setSegmentContexts(segmentContexts);
+      return;
+    }
+    if (_consistencyMode == UpsertConfig.ConsistencyMode.SYNC) {
+      _upsertViewLock.readLock().lock();
+      try {
+        setSegmentContexts(segmentContexts);
+        return;
+      } finally {
+        _upsertViewLock.readLock().unlock();
+      }
+    }
+    // If batch refresh is enabled, the copy of bitmaps is kept updated and ready to use for a consistent view.
+    // The locking between query threads and upsert threads can be avoided when using batch refresh.
+    // Besides, queries can share the copy of bitmaps, w/o cloning the bitmaps by every single query.
+    // If query has specified a need for certain freshness, check the view and refresh it as needed.
+    // When refreshing the copy of map, we need to take the WLock so only one thread is refreshing view.
+    long upsertViewFreshnessMs =
+        Math.min(QueryOptionsUtils.getUpsertViewFreshnessMs(queryOptions), _upsertViewRefreshIntervalMs);
+    if (upsertViewFreshnessMs == -1) {

Review Comment:
   More robust
   ```suggestion
       if (upsertViewFreshnessMs < 0) {
   ```



-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org