snleee commented on a change in pull request #8422:
URL: https://github.com/apache/pinot/pull/8422#discussion_r839119100
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,16 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager,
offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource",
offlineTableName);
+ // Drop the table on servers
+ // TODO: Wait for externalview to converge on controllers instead of
servers. This is because if externalview
+ // gets updated with significant delay. We may have the race
condition for table recreation that
Review comment:
`delay. We -> delay, we`
##########
File path:
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1757,6 +1758,16 @@ public void deleteOfflineTable(String tableName) {
HelixHelper.removeResourceFromBrokerIdealState(_helixZkManager,
offlineTableName);
LOGGER.info("Deleting table {}: Removed from broker resource",
offlineTableName);
+ // Drop the table on servers
+ // TODO: Wait for externalview to converge on controllers instead of
servers. This is because if externalview
Review comment:
Can you also mention that we should make this API to be blocking and we
also need to make this to be idempotent?
##########
File path:
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java
##########
@@ -73,6 +75,9 @@
@ThreadSafe
public class HelixInstanceDataManager implements InstanceDataManager {
private static final Logger LOGGER =
LoggerFactory.getLogger(HelixInstanceDataManager.class);
+ // TODO: Make this configurable
+ private static final long EXTERNAL_VIEW_DROPPED_MAX_WAIT_MS = 20 * 60_000L;
// 20 minutes
Review comment:
What is our default value for the merge and rollup on the segment
replacement protocol?
##########
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:
If make the drop table blocking, we probably don't need to wait (drop
table will make sure to wait to converge before deleting if there's a
difference between idealstate & external view).
Let's add `TODO` comment as well to remove this once we make delete API to
be blocking
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
