date:20240610

Re: [PR] Add list of collaborators to asf.yaml [pinot]

2024-06-10 Thread via GitHub



yashmayya commented on PR #13346:
URL: https://github.com/apache/pinot/pull/13346#issuecomment-2159769737

   Thanks Jackie! Mentioning the newly added collaborators here as an FYI - 
@sullis, @shenyu0127, @tibrewalpratik17, @abhioncbr, @zhtaoxiang, 
@shounakmk219, @itschrispeck, @soumitra-st, @swaminathanmanish, @yashmayya. 


-- 
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-unsubscr...@pinot.apache.org

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


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

Re: [PR] Optimize snapshot flow to only snapshot segments which have updates [pinot]

2024-06-10 Thread via GitHub



klsince commented on code in PR #13285:
URL: https://github.com/apache/pinot/pull/13285#discussion_r1634137440


##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##
@@ -703,9 +703,20 @@ public void run() {
 //   persisted.
 // Take upsert snapshot before starting consuming events
 if (_partitionUpsertMetadataManager != null) {
-  _partitionUpsertMetadataManager.takeSnapshot();
-  // If upsertTTL is enabled, we will remove expired primary keys from 
upsertMetadata after taking snapshot.
-  _partitionUpsertMetadataManager.removeExpiredPrimaryKeys();
+  if (_tableConfig.getUpsertMetadataTTL() > 0) {
+// If upsertMetadataTTL is enabled, we will remove expired primary 
keys from upsertMetadata
+// AFTER taking a snapshot. Taking the snapshot first is crucial 
to ensure we capture the final
+// state of a particular key before it exits the TTL window.

Review Comment:
   I see, that makes sense. Based on the metadata TTL related code, it seems 
like taking snapshot after removing metadata out of the TTL wouldn’t affect 
data/query correctness but incurred some extra overhead if server restarted 
before taking new snapshot as it had to add metadata (already out of TTL) back 
to Map, however those metadata would be removed again. 
   
   IIUC, maybe update the comment a bit that this is mainly to avoid such 
overhead upon unexpected server restart, but not affecting correctness. 
   



-- 
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-unsubscr...@pinot.apache.org

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


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

Re: [PR] fix race condition in `ScalingThreadPoolExecutor` [pinot]

2024-06-10 Thread via GitHub



codecov-commenter commented on PR #13360:
URL: https://github.com/apache/pinot/pull/13360#issuecomment-2159718614

   ## 
[Codecov](https://app.codecov.io/gh/apache/pinot/pull/13360?dropdown=coverage=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 Report
   Attention: Patch coverage is `0%` with `10 lines` in your changes missing 
coverage. Please review.
   > Project coverage is 0.00%. Comparing base 
[(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 to head 
[(`98eb469`)](https://app.codecov.io/gh/apache/pinot/commit/98eb46996f85598df92283088456109e7af76f8e?dropdown=coverage=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache).
   > Report is 599 commits behind head on master.
   
   | 
[Files](https://app.codecov.io/gh/apache/pinot/pull/13360?dropdown=coverage=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | Patch % | Lines |
   |---|---|---|
   | 
[.../pinot/common/utils/ScalingThreadPoolExecutor.java](https://app.codecov.io/gh/apache/pinot/pull/13360?src=pr=tree=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Futils%2FScalingThreadPoolExecutor.java_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvU2NhbGluZ1RocmVhZFBvb2xFeGVjdXRvci5qYXZh)
 | 0.00% | [10 Missing :warning: 
](https://app.codecov.io/gh/apache/pinot/pull/13360?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 |
   
   Additional details and impacted files
   
   
   ```diff
   @@  Coverage Diff  @@
   ## master   #13360   +/-   ##
   =
   - Coverage 61.75%0.00%   -61.76% 
   =
 Files  2436 2472   +36 
 Lines133233   136271 +3038 
 Branches  2063621166  +530 
   =
   - Hits  822740-82274 
   - Misses44911   136271+91360 
   + Partials   60480 -6048 
   ```
   
   | 
[Flag](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `?` | |
   | 
[integration](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
   | 
[integration1](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `?` | |
   | 
[integration2](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `0.00% <0.00%> (ø)` | |
   | 
[java-11](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `?` | |
   | 
[java-21](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `0.00% <0.00%> (-61.63%)` | :arrow_down: |
   | 
[skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `0.00% <0.00%> (-61.75%)` | :arrow_down: |
   | 
[skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `?` | |
   | 
[temurin](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `0.00% <0.00%> (-61.76%)` | :arrow_down: |
   | 
[unittests](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `?` | |
   | 
[unittests1](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `?` | |
   | 
[unittests2](https://app.codecov.io/gh/apache/pinot/pull/13360/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache)
 | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click

(pinot) branch master updated (ad5ca349ab -> d09cd0cec7)

2024-06-10 Thread xbli

This is an automated email from the ASF dual-hosted git repository.

xbli pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git


from ad5ca349ab Add list of collaborators to asf.yaml (#13346)
 add d09cd0cec7 move shouldReplaceOnComparisonTie to base class to be more 
reusable (#13353)

No new revisions were added by this update.

Summary of changes:
 .../upsert/BasePartitionUpsertMetadataManager.java | 42 +
 ...oncurrentMapPartitionUpsertMetadataManager.java | 43 --
 2 files changed, 42 insertions(+), 43 deletions(-)


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

Re: [PR] move shouldReplaceOnComparisonTie to base class to be more reusable [pinot]

2024-06-10 Thread via GitHub



klsince merged PR #13353:
URL: https://github.com/apache/pinot/pull/13353


-- 
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-unsubscr...@pinot.apache.org

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


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

Re: [PR] extend CompactedPinotSegmentRecordReader so that it can skip deleteRecord [pinot]

2024-06-10 Thread via GitHub



klsince commented on code in PR #13352:
URL: https://github.com/apache/pinot/pull/13352#discussion_r1634119955


##
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/CompactedPinotSegmentRecordReader.java:
##
@@ -36,19 +37,25 @@
 public class CompactedPinotSegmentRecordReader implements RecordReader {
   private final PinotSegmentRecordReader _pinotSegmentRecordReader;
   private final RoaringBitmap _validDocIdsBitmap;
-
+  private final String _deleteRecordColumn;
+  // Reusable generic row to store the next row to return
+  private final GenericRow _nextRow = new GenericRow();
   // Valid doc ids iterator
   private PeekableIntIterator _validDocIdsIterator;
-  // Reusable generic row to store the next row to return
-  private GenericRow _nextRow = new GenericRow();
   // Flag to mark whether we need to fetch another row
   private boolean _nextRowReturned = true;
 
   public CompactedPinotSegmentRecordReader(File indexDir, RoaringBitmap 
validDocIds) {
+this(indexDir, validDocIds, null);
+  }
+
+  public CompactedPinotSegmentRecordReader(File indexDir, RoaringBitmap 
validDocIds,
+  @Nullable String deleteRecordColumn) {

Review Comment:
   As it’s not safe to skip deleteRecord, so I would rather avoid it for less 
confusion. 



-- 
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-unsubscr...@pinot.apache.org

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


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

[PR] fix race condition in `ScalingThreadPoolExecutor` [pinot]

2024-06-10 Thread via GitHub

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

I introduced `ScalingThreadPoolExecutor` previously to provide an
autoscaling thread pool, used to prevent interrupts from corrupting the
realtime Lucene index.

There is a race condition as the logic relied on `_executor.getPoolSize()`
and `_executor.getActiveCount()`. This PR changes the implementation slightly
to track idle threads via overriding the [two methods that may be used by
ThreadPoolExecutor.getTask()](https://github.com/openjdk/jdk21/blob/890adb6410dab4606a4f26a942aed02fb2f55387/src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java#L1069-L1070),
which is always executed by an idle thread to pick up the next task.

In theory this could cause sporadic timeouts for searches against the
realtime Lucene index, though it is hard to reproduce. I came across this bug
trying to use `ScalingThreadPoolExecutor` for another feature.

For testing, unit tests should cover this logic. The race condition is
easily reproducible when the added unit test is used against the old
implementation.

suggested tag: `bugfix`

--
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.

87 matches

Mail list logo