[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r267867758
 
 

 ##
 File path: 
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);
 
 Review comment:
   Done.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r267806504
 
 

 ##
 File path: 
pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java
 ##
 @@ -156,6 +165,35 @@ public void removeSegment(@Nonnull String segmentName) {
 }
   }
 
+  /**
+   * Called when a segment is deleted. The actual handling of segment delete 
is outside of this method.
+   * This method provides book-keeping around deleted segments.
+   * @param segmentName name of the segment to track.
+   */
+  public void trackDeletedSegment(@Nonnull String segmentName) {
+// add segment to the cache
+_deletedSegmentsCache.put(segmentName, true);
+  }
+
+  /**
+   * Check if a segment is recently deleted.
+   *
+   * @param segmentName name of the segment to check.
+   * @return true if segment is in the cache, false otherwise
+   */
+  public boolean isRecentlyDeleted(@Nonnull String segmentName) {
+return _deletedSegmentsCache.getIfPresent(segmentName) != null;
+  }
+
+  /**
+   * Remove a segment from the deleted cache if it is being added back.
+   *
+   * @param segmentName name of the segment that needs to removed from the 
cache (if needed)
+   */
+  private void untrackIfDeleted(@Nonnull String segmentName) {
 
 Review comment:
   I am ambivalent about this. If you feel strongly, I can change it. Will see 
if Subbu has any input.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r266107313
 
 

 ##
 File path: 
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,
 
 Review comment:
   Done.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r266107271
 
 

 ##
 File path: 
pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java
 ##
 @@ -124,7 +125,22 @@ public DataTable processQuery(ServerQueryRequest 
queryRequest, ExecutorService e
 
 TableDataManager tableDataManager = 
_instanceDataManager.getTableDataManager(tableNameWithType);
 Preconditions.checkState(tableDataManager != null, "Failed to find data 
manager for table: " + tableNameWithType);
-List segmentDataManagers = 
tableDataManager.acquireSegments(queryRequest.getSegmentsToQuery());
+
+// acquire the segments
+int missingSegments = 0;
+List segmentsToQuery = queryRequest.getSegmentsToQuery();
+List segmentDataManagers = new ArrayList<>();
+for (String segmentName : segmentsToQuery) {
+  SegmentDataManager segmentDataManager = 
tableDataManager.acquireSegment(segmentName);
+  if (segmentDataManager != null) {
+segmentDataManagers.add(segmentDataManager);
+  } else {
+if (!tableDataManager.isDeleted(segmentName)) {
 
 Review comment:
   Renamed.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r266107378
 
 

 ##
 File path: 
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);
 
 Review comment:
   Removed log.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r266107218
 
 

 ##
 File path: 
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;
 
 Review comment:
   Updated.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r266107149
 
 

 ##
 File path: 
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.
+   */
+  void deleteSegment(@Nonnull String segmentName);
 
 Review comment:
   Done. Renamed to trackDeletedSegment


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r265736137
 
 

 ##
 File path: 
pinot-server/src/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);
 
 Review comment:
   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'.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r265735525
 
 

 ##
 File path: 
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,
 
 Review comment:
   Good point; will add it around line 138


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[GitHub] [incubator-pinot] sunithabeeram commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired

[GitBox]

sunithabeeram commented on a change in pull request #3942: Set 
processingException when all queried segments cannot be acquired
URL: https://github.com/apache/incubator-pinot/pull/3942#discussion_r265735249
 
 

 ##
 File path: 
pinot-core/src/main/java/org/apache/pinot/core/data/manager/InstanceDataManager.java
 ##
 @@ -78,6 +78,13 @@ void addRealtimeSegment(@Nonnull String realtimeTableName, 
@Nonnull String segme
   void removeSegment(@Nonnull String tableNameWithType, @Nonnull String 
segmentName)
   throws Exception;
 
+  /**
+   * Handles deletion of a segment from the table.
+   *
+   * This method performs book keeping of deleted segments.
+   */
+  void deleteSegment(@Nonnull String tableNameWithType, @Nonnull String 
segmentName);
 
 Review comment:
   Will change the method name.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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