snleee commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r838865953
##########
File path:
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java
##########
@@ -408,7 +413,41 @@ public void testUploadSegmentRefreshOnly()
assertEquals(segmentsZKMetadata.size(), 1);
assertNotEquals(segmentsZKMetadata.get(0).getRefreshTime(),
Long.MIN_VALUE);
}
+ waitForNumOfSegmentsBecomeOnline(offlineTableName, 1);
Review comment:
What happens if we remove this?
##########
File path:
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerMeter.java
##########
@@ -33,6 +33,7 @@
QUERY_EXECUTION_EXCEPTIONS("exceptions", false),
HELIX_ZOOKEEPER_RECONNECTS("reconnects", true),
DELETED_SEGMENT_COUNT("segments", false),
+ DELETE_TABLE_FAILURES("tables", true),
Review comment:
are we emitting the metrics per table? In that case, does this needs to
be `false`? For instance, refresh failure sets this to be `false`. Can you
double-check on this?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,10 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager,
offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource",
offlineTableName);
+ // Drop the table on servers
+ deleteTableOnServer(offlineTableName);
Review comment:
I see that we don't send the message if `externalview` is null from
`deleteTableOnServer()`. If we send without filtering, the helix side will
throw the exception? Or, you mention that we explicitly throw the exception?
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -3285,10 +3324,11 @@ private void waitForSegmentsBecomeOnline(String
tableNameWithType, Set<String> s
tableNameWithType, segmentsToCheck));
}
- private Set<String> getOnlineSegmentsFromExternalView(String
tableNameWithType) {
+ public Set<String> getOnlineSegmentsFromExternalView(String
tableNameWithType) {
ExternalView externalView = getTableExternalView(tableNameWithType);
- Preconditions
- .checkState(externalView != null, String.format("External view is null
for table (%s)", tableNameWithType));
+ if (externalView == null) {
Review comment:
Have you checked all the usage of `getOnlineSegmentsFromExternalView`?
This will change the behavior (returning null instead of throwing the
exception).
--
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
