[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r267811436
##
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:
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.
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] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r267808241
##
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:
Or simply, notifySegmentDeleted() and notifySegmentAdded()?
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] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r265634226
##
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:
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
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] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r265633589 ## 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: same with this method name. It is not really deleting a segment, is it? 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] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r265635164 ## 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: 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 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] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r265630415 ## 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: +1 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] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r265635793 ## 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: Also, can we not get the table data manager directly from here? Do we need to go through the instanceDataManager? Dependency issues? 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] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r265631366
##
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:
isRecentlyDeleted()?
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] mcvsubbu commented on a change in pull request #3942: Set processingException when all queried segments cannot be acquired
mcvsubbu 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_r265631787
##
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:
It may be worth adding a log message here with the name of the segments
missing, Can help us debug
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
