Set processingException when all queried segments cannot be acquired #3942

sunithabeeram · 2019-03-08T22:56:50Z

Motivation: We have seen cases where due to bugs or delays wrt EV processing on the broker, broker sends a list of segments to the servers that are not hosted by it. This is technically a partial-response, but we currently don't indicate it as such. We would like to track this to the most extent possible.

Details: There are cases where segments can be legitimately missing on the server; the known reason for this is when segments are deleted (either through retention or manually) and the broker's EV isn't updated yet. To not count partial responses in this scenario, we maintain an in-memory cache of deleted segment names. If a recently deleted segment is queried, we check our cache and not count it as an error if the segment-name exists in the cache.
NOTE: This can still miss cases where the server is restarted in the window between deletion and broker EV update. However, the chances of this are deemed low (and cost of catching this error deemed high).

Testing done: Added unit tests to cover the new paths.

Impact: Once this change is rolled out, num-segments-missing server metric should report lower numbers; we should see partial-responses broker metric go up if we hit missing-segments issue on the server.

Please see #4007 for details of the broader effort for this PR.

codecov-io · 2019-03-08T23:33:24Z

Codecov Report

Merging #3942 into master will decrease coverage by 20.96%.
The diff coverage is 87.09%.

@@              Coverage Diff              @@
##             master    #3942       +/-   ##
=============================================
- Coverage     67.23%   46.26%   -20.97%     
=============================================
  Files          1032     1032               
  Lines         51147    51172       +25     
  Branches       7155     7161        +6     
=============================================
- Hits          34387    23677    -10710     
- Misses        14382    25470    +11088     
+ Partials       2378     2025      -353

Impacted Files	Coverage Δ	Complexity Δ
.../apache/pinot/common/exception/QueryException.java	`88.57% <100%> (+0.16%)`	`0 <0> (ø)`	⬇️
...r/helix/SegmentOnlineOfflineStateModelFactory.java	`67.96% <100%> (+0.63%)`	`0 <0> (ø)`	⬇️
...server/starter/helix/HelixInstanceDataManager.java	`85.6% <75%> (-0.69%)`	`0 <0> (ø)`
.../pinot/core/data/manager/BaseTableDataManager.java	`76.82% <83.33%> (-17%)`	`0 <0> (ø)`
...core/query/executor/ServerQueryExecutorV1Impl.java	`79.5% <92.85%> (+0.6%)`	`0 <0> (ø)`	⬇️
.../org/apache/pinot/common/http/MultiGetRequest.java	`0% <0%> (-100%)`	`0% <0%> (ø)`
...che/pinot/common/restlet/resources/TablesList.java	`0% <0%> (-100%)`	`0% <0%> (ø)`
...che/pinot/pql/parsers/pql2/ast/OptionsAstNode.java	`0% <0%> (-100%)`	`0% <0%> (ø)`
...ers/pql2/ast/PredicateParenthesisGroupAstNode.java	`0% <0%> (-100%)`	`0% <0%> (ø)`
...ot/core/query/scheduler/TableBasedGroupMapper.java	`0% <0%> (-100%)`	`0% <0%> (ø)`
... and 523 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfaaf11...ee244c5. Read the comment docs.

snleee

LGTM otherwise.

snleee · 2019-03-14T04:26:22Z

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

+  /**
+   * Track a deleted segment.
+   */
+  void deleteSegment(@Nonnull String segmentName);


Can we rename this function? deleteSegment sounds like we are deleting segments instead of book-keeping. It's confusing since we have another method called removeSegment.

Done. Renamed to trackDeletedSegment

snleee · 2019-03-14T04:28:05Z

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

@@ -42,9 +44,13 @@
 @ThreadSafe
 public abstract class BaseTableDataManager implements TableDataManager {
  private static final Logger LOGGER = LoggerFactory.getLogger(BaseTableDataManager.class);
+  // cache atmost a given max of deleted segments
+  private static final int MAX_DELETED_SEGMENT_CACHE_SIZE = 1000;


Discussed offline. We can probably add the time based eviction and increase MAX_DELETED_SEGMENT_CACHE_SIZE.

mcvsubbu · 2019-03-14T15:35:14Z

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

+  /**
+   * Track a deleted segment.
+   */
+  void deleteSegment(@Nonnull String segmentName);


mcvsubbu · 2019-03-14T15:37:02Z

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

+      if (segmentDataManager != null) {
+        segmentDataManagers.add(segmentDataManager);
+      } else {
+        if (!tableDataManager.isDeleted(segmentName)) {


isRecentlyDeleted()?

mcvsubbu · 2019-03-14T15:37:52Z

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

@@ -195,6 +211,13 @@ public DataTable processQuery(ServerQueryRequest queryRequest, ExecutorService e
    long queryProcessingTime = queryProcessingTimer.getDurationMs();
    dataTable.getMetadata().put(DataTable.NUM_SEGMENTS_QUERIED, Integer.toString(numSegmentsQueried));
    dataTable.getMetadata().put(DataTable.TIME_USED_MS_METADATA_KEY, Long.toString(queryProcessingTime));
+
+    if (missingSegments > 0) {
+      dataTable.addException(QueryException.getException(QueryException.SEGMENTS_MISSING_ERROR,


It may be worth adding a log message here with the name of the segments missing, Can help us debug

Good point; will add it around line 138

mcvsubbu · 2019-03-14T15:41:29Z

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

+   *
+   * This method performs book keeping of deleted segments.
+   */
+  void deleteSegment(@Nonnull String tableNameWithType, @Nonnull String segmentName);


same with this method name. It is not really deleting a segment, is it?

Will change the method name.

mcvsubbu · 2019-03-14T15:42:44Z

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java

@@ -151,6 +151,15 @@ public void removeSegment(@Nonnull String tableNameWithType, @Nonnull String seg
    }
  }

+  @Override
+  public void deleteSegment(@Nonnull String tableNameWithType, @Nonnull String segmentName) {
+    LOGGER.info("Tracking deleted segment: {} from table: {}", segmentName, tableNameWithType);


I think we have other logs for dropping a segment, do we need this one as well? If we really do, then it should go inside the 'if' statement

Removed log.

mcvsubbu · 2019-03-14T15:44:42Z

...c/main/java/org/apache/pinot/server/starter/helix/SegmentOnlineOfflineStateModelFactory.java

@@ -196,6 +196,8 @@ public void onBecomeDroppedFromOffline(Message message, NotificationContext cont
      String tableNameWithType = message.getResourceName();
      String segmentName = message.getPartitionName();

+      _instanceDataManager.deleteSegment(tableNameWithType, segmentName);


This line should be inserted after line 184, not here. The segment is not available for any new queries sometime after line 184 starts execution

Also, can we not get the table data manager directly from here? Do we need to go through the instanceDataManager? Dependency issues?

After line 184, the segment is unavailable correct; and if it is due to a move/rebalance, we should count it as an error. We only want to suppress it when the segment has been 'deleted'.

kishoreg · 2019-03-14T16:09:51Z

Agree with comments. Looking at the existing code, I felt it could be handled within BaseTableDataManager - have the cache (you have it), just update it in removeSegment and use that in handleMissingSegment (don't increment the metric). Am I missing something?

snleee

LGTM otherwise.

Another edge case that can be covered in the future.

explicitly delete a segment
upload the new segment with the same name
that segment becomes missing

In the above case, we won't report the segment as missing because the cache will still contain the recently deleted segments (for 6hrs). For completeness, I think that we should also cover that case.

snleee · 2019-03-20T22:56:56Z

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

@@ -79,6 +79,16 @@ void addSegment(@Nonnull String segmentName, @Nonnull TableConfig tableConfig,
   */
  void removeSegment(@Nonnull String segmentName);

+  /**
+   * Track a deleted segment.


can we add @param here to follow javadoc convention?

mcvsubbu

We have a table config that disables message-based refresh for refresh use case. In this case, the old segment is dropped and a new segment is created with the same name.
See PinotHelixManager.refreshSegment().
In this case, we should be removing a newly added segment from the deleted tracker cache? The deletion tracker is getting out of hand, but I don't have an alternative solution.

Also, we need to make a note somewhere that the deletion cache is unreliable (i.e. we lose the cache upon restart). I suggest in ServerQueryExecutorV1Impl (unless we decide to persist the deletion cache....)

snleee · 2019-03-21T02:17:58Z

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

+   *
+   * @param segmentName name of the segment that needs to removed from the cache (if needed)
+   */
+  private void untrackIfDeleted(@Nonnull String segmentName) {


In my opinion, untrackDeletedSegment() seems to be better.

I am ambivalent about this. If you feel strongly, I can change it. Will see if Subbu has any input.

Or simply, notifySegmentDeleted() and notifySegmentAdded()?

I think thats a better a choice. Thanks for the suggestion!

mcvsubbu · 2019-03-21T15:16:28Z

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

+   *
+   * @param segmentName name of the segment that needs to removed from the cache (if needed)
+   */
+  private void untrackIfDeleted(@Nonnull String segmentName) {


Or simply, notifySegmentDeleted() and notifySegmentAdded()?

mcvsubbu · 2019-03-21T15:22:58Z

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

@@ -110,6 +118,7 @@ public void shutDown() {
  @Override
  public void addSegment(@Nonnull ImmutableSegment immutableSegment) {
    String segmentName = immutableSegment.getSegmentName();
+    untrackIfDeleted(segmentName);


This line should move down to be after line 128 (when we populate the _segmentDataManagerMap). The segment is available for query only after that point.

…ect to tracking segments added and deleted

…cquired (#3942)" This reverts commit d4f2ece.

…cquired (#3942)" (#4632) This reverts commit d4f2ece.

…cquired (apache#3942)" (apache#4632) This reverts commit d4f2ece.

snleee approved these changes Mar 14, 2019

View reviewed changes

mcvsubbu reviewed Mar 14, 2019

View reviewed changes

snleee approved these changes Mar 20, 2019

View reviewed changes

mcvsubbu reviewed Mar 20, 2019

View reviewed changes

snleee reviewed Mar 21, 2019

View reviewed changes

mcvsubbu reviewed Mar 21, 2019

View reviewed changes

Sunitha Beeram added 9 commits March 21, 2019 11:32

Set processingException when all queried segments cannot be acquired

ba4bff6

Minor fixes

6436188

Address review comments

833a72d

Disable setting processing exception until metrics are validated

a2e5c54

Address review comments

b3a63ac

Address review comments

7968f0b

Address review comments

1decb81

Address review comments

8475d04

Address review comments: Make InstanceDataManager symmetric with resp…

ee244c5

…ect to tracking segments added and deleted

sunithabeeram force-pushed the trackDeletedSegments branch from e522b2c to ee244c5 Compare March 21, 2019 18:32

mcvsubbu approved these changes Mar 21, 2019

View reviewed changes

sunithabeeram mentioned this pull request Mar 22, 2019

Improve/Add completeness and freshness metrics for Pinot #4007

Open

sunithabeeram merged commit d4f2ece into apache:master Mar 22, 2019

sunithabeeram deleted the trackDeletedSegments branch March 22, 2019 16:33

jenniferdai added a commit that referenced this pull request Sep 23, 2019

Revert "Set processingException when all queried segments cannot be a…

353c7b1

…cquired (#3942)" This reverts commit d4f2ece.

jenniferdai mentioned this pull request Sep 23, 2019

Revert "Set processingException when all queried segments cannot be a… #4632

Merged

jenniferdai added a commit that referenced this pull request Sep 24, 2019

Revert "Set processingException when all queried segments cannot be a…

283a495

…cquired (#3942)" (#4632) This reverts commit d4f2ece.

chenboat pushed a commit to chenboat/incubator-pinot that referenced this pull request Nov 15, 2019

Revert "Set processingException when all queried segments cannot be a…

8b37fbc

…cquired (apache#3942)" (apache#4632) This reverts commit d4f2ece.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set processingException when all queried segments cannot be acquired #3942

Set processingException when all queried segments cannot be acquired #3942

sunithabeeram commented Mar 8, 2019 •

edited

codecov-io commented Mar 8, 2019 •

edited

snleee left a comment

snleee Mar 14, 2019

mcvsubbu Mar 14, 2019

sunithabeeram Mar 15, 2019

snleee Mar 14, 2019

sunithabeeram Mar 15, 2019

mcvsubbu Mar 14, 2019

mcvsubbu Mar 14, 2019

sunithabeeram Mar 15, 2019

mcvsubbu Mar 14, 2019

sunithabeeram Mar 14, 2019

sunithabeeram Mar 15, 2019

mcvsubbu Mar 14, 2019

sunithabeeram Mar 14, 2019

mcvsubbu Mar 14, 2019

sunithabeeram Mar 15, 2019

mcvsubbu Mar 14, 2019

mcvsubbu Mar 14, 2019

sunithabeeram Mar 14, 2019

kishoreg commented Mar 14, 2019

snleee left a comment

snleee Mar 20, 2019

mcvsubbu left a comment

snleee Mar 21, 2019

sunithabeeram Mar 21, 2019

mcvsubbu Mar 21, 2019

sunithabeeram Mar 21, 2019

mcvsubbu Mar 21, 2019

mcvsubbu Mar 21, 2019

sunithabeeram Mar 21, 2019

Set processingException when all queried segments cannot be acquired #3942

Set processingException when all queried segments cannot be acquired #3942

Conversation

sunithabeeram commented Mar 8, 2019 • edited

codecov-io commented Mar 8, 2019 • edited

Codecov Report

snleee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishoreg commented Mar 14, 2019

snleee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcvsubbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunithabeeram commented Mar 8, 2019 •

edited

codecov-io commented Mar 8, 2019 •

edited