Cache Deleted Segment Names in Server to Avoid SegmentMissingError #9423

ankitsultana · 2022-09-19T00:51:44Z

Added a UT. fixes #9371

ankitsultana · 2022-09-19T00:55:30Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+   */
+  @Nullable
+  protected SegmentDataManager registerSegment(String segmentName, SegmentDataManager segmentDataManager) {
+    SegmentDataManager oldSegmentDataManager = _segmentDataManagerMap.put(segmentName, segmentDataManager);


I think we may wanna make _segmentDataManagerMap private so concrete classes that extend BaseTableDataManager use the map only via the provided methods here.

codecov-commenter · 2022-09-19T01:39:29Z

Codecov Report

Merging #9423 (13c4a37) into master (3633495) will increase coverage by 0.05%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master    #9423      +/-   ##
============================================
+ Coverage     69.73%   69.78%   +0.05%     
- Complexity     5087     5094       +7     
============================================
  Files          1890     1890              
  Lines        100743   100780      +37     
  Branches      15349    15351       +2     
============================================
+ Hits          70252    70333      +81     
+ Misses        25506    25450      -56     
- Partials       4985     4997      +12

Flag	Coverage Δ
integration1	`26.01% <47.22%> (+0.03%)`	⬆️
integration2	`24.65% <50.00%> (-0.07%)`	⬇️
unittests1	`67.00% <67.64%> (+<0.01%)`	⬆️
unittests2	`15.37% <0.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ent/local/data/manager/TableDataManagerConfig.java	`0.00% <0.00%> (ø)`
...core/query/executor/ServerQueryExecutorV1Impl.java	`81.23% <84.61%> (-0.32%)`	⬇️
.../pinot/core/data/manager/BaseTableDataManager.java	`74.18% <92.30%> (+0.62%)`	⬆️
...ata/manager/realtime/RealtimeTableDataManager.java	`68.19% <100.00%> (-0.77%)`	⬇️
.../starter/helix/HelixInstanceDataManagerConfig.java	`81.25% <100.00%> (+0.81%)`	⬆️
...requesthandler/MultiStageBrokerRequestHandler.java	`59.15% <0.00%> (-7.90%)`	⬇️
.../common/request/context/predicate/EqPredicate.java	`92.30% <0.00%> (-7.70%)`	⬇️
...inot/core/util/SegmentCompletionProtocolUtils.java	`57.69% <0.00%> (-7.70%)`	⬇️
...or/transform/function/IsNullTransformFunction.java	`78.57% <0.00%> (-7.15%)`	⬇️
...tream/kafka20/server/KafkaDataServerStartable.java	`72.91% <0.00%> (-6.25%)`	⬇️
... and 34 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jackjlli

I feel it's a legitimate behavior that server finished segment deletion before controller updates the EV, as controller won't update it until it receives the message from servers saying the state transition is completed, not to mention brokers listen to the notification of EV changes to update routing table, which is the very last step.

General speaking I think the better way (or best practice) is to avoid querying the very last pieces of segments, by adding the time column in your filter clauses of the queries. If the Pinot table you are querying is a hybrid table, the oldest realtime segments won't be queried because of the correct time boundary maintained internally. Thus we won't see such exception even if we enable the exception returned feature that you implemented. Similarly, if we only query the realtime only or offline only table, we shouldn't query the oldest segments, as the behavior of them is harder to predict in a distributed world. Adding such cache will definitely work while it seems more like a hack to get around with it.

pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java

jackjlli · 2022-09-19T18:24:58Z

...rver/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManagerConfig.java

  private final static String[] REQUIRED_KEYS = {INSTANCE_ID, INSTANCE_DATA_DIR, READ_MODE};
  private static final long DEFAULT_ERROR_CACHE_SIZE = 100L;
+  private static final int DEFAULT_DELETED_SEGMENTS_CACHE_SIZE = 10_000;
+  private static final int DEFAULT_DELETED_SEGMENTS_CACHE_TTL_MINUTES = 10;


Usually the state transition doesn't take too much time (at most sub-seconds) on spreading the helix messages and notifications. If it really needs 10 minutes to converge, it probably means there is something wrong in your cluster and needs to be fixed.

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

ankitsultana · 2022-09-19T18:45:56Z

General speaking I think the better way (or best practice) is to avoid querying the very last pieces of segments, by adding the time column in your filter clauses of the queries.

This is discussed in #9371. It doesn't handle the scenario where a segment is manually deleted. Requiring users to add filters in the query isn't tenable, and the alternative is to automatically prune such segments in the Broker. But that's not doable right now since Broker doesn't watch TableConfig changes (retention may be changed by users).

jackjlli · 2022-09-19T18:59:26Z

It doesn't handle the scenario where a segment is manually deleted.

I don't think it's a good idea to manually delete segments when segments are being queried. The better way is to disable them before doing the deletion.

Requiring users to add filters in the query isn't tenable.

I'm not saying we should ask users to do that but your client system append the predicate in the filter clause.

and the alternative is to automatically prune such segments in the Broker. But that's not doable right now since Broker doesn't watch TableConfig changes (retention may be changed by users).

I'm not sure I fully understand your statement here. Pruning segments has nothing to do with table config changes. And in fact table config is already cached in broker, and there is a watcher in TableCache to listen to the table config changes in ZK.

ankitsultana · 2022-09-19T19:07:26Z

I'm not sure I fully understand your statement here. Pruning segments has nothing to do with table config changes. And in fact table config is already cached in broker, and there is a watcher in TableCache to listen the table config changes in ZK.

For instance when a user changes the retention, it may take a while for the changes to be reflected in Broker segment pruners. @Jackie-Jiang: I think you mentioned that this scenario may cause an issue since Broker doesn't listen to table-config changes. Can you share those concerns here?

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java

The PR still needs unit tests.

jackjlli

Minor but LGTM. Thanks for addressing it!

jackjlli · 2022-09-21T20:56:35Z

pinot-spi/src/main/java/org/apache/pinot/spi/config/instance/InstanceDataManagerConfig.java


  long getStreamSegmentDownloadUntarRateLimit();
+
+  int getDeletedSegmentsCacheSize();


nit: getRecentlyDeletedSegmentsCacheSize()

jackjlli · 2022-09-21T20:56:41Z

pinot-spi/src/main/java/org/apache/pinot/spi/config/instance/InstanceDataManagerConfig.java

+
+  int getDeletedSegmentsCacheSize();
+
+  int getDeletedSegmentsCacheTtlMinutes();


…pache#9423) * Cache Deleted Segment Names in Server to Avoid SegmentMissingError * self-review: remove unneeded call + change type for numMissingSegments * Address feedback * Add UT + Address Feedback

Cache Deleted Segment Names in Server to Avoid SegmentMissingError

113fc95

ankitsultana commented Sep 19, 2022

View reviewed changes

self-review: remove unneeded call + change type for numMissingSegments

3c005f8

mcvsubbu requested a review from jackjlli September 19, 2022 15:45

jackjlli reviewed Sep 19, 2022

View reviewed changes

Address feedback

13c4a37

chenboat previously approved these changes Sep 20, 2022

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java Outdated Show resolved Hide resolved

pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java Outdated Show resolved Hide resolved

chenboat reviewed Sep 20, 2022

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java Outdated Show resolved Hide resolved

chenboat self-requested a review September 20, 2022 18:29

Add UT + Address Feedback

5fd156a

ankitsultana requested review from chenboat and jackjlli and removed request for chenboat and jackjlli September 21, 2022 19:58

jackjlli approved these changes Sep 21, 2022

View reviewed changes

chenboat approved these changes Sep 22, 2022

View reviewed changes

chenboat merged commit aa1d2f6 into apache:master Sep 22, 2022


		long getStreamSegmentDownloadUntarRateLimit();

		int getDeletedSegmentsCacheSize();


		int getDeletedSegmentsCacheSize();

		int getDeletedSegmentsCacheTtlMinutes();

Cache Deleted Segment Names in Server to Avoid SegmentMissingError #9423

Cache Deleted Segment Names in Server to Avoid SegmentMissingError #9423

Uh oh!

Conversation

ankitsultana commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankitsultana Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jackjlli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jackjlli Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ankitsultana commented Sep 19, 2022

Uh oh!

jackjlli commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ankitsultana commented Sep 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jackjlli left a comment

Choose a reason for hiding this comment

Uh oh!

jackjlli Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

jackjlli Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ankitsultana commented Sep 19, 2022 •

edited

Loading

codecov-commenter commented Sep 19, 2022 •

edited

Loading

jackjlli commented Sep 19, 2022 •

edited

Loading