Jackie-Jiang commented on code in PR #9306:
URL: https://github.com/apache/pinot/pull/9306#discussion_r995111114
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/SegmentOnlineOfflineStateModelFactory.java:
##########
@@ -200,22 +196,11 @@ public void onBecomeDroppedFromOffline(Message message,
NotificationContext cont
_logger.info("SegmentOnlineOfflineStateModel.onBecomeDroppedFromOffline() : " +
message);
String tableNameWithType = message.getResourceName();
String segmentName = message.getPartitionName();
-
- // This method might modify the file on disk. Use segment lock to
prevent race condition
- Lock segmentLock = SegmentLocks.getSegmentLock(tableNameWithType,
segmentName);
Review Comment:
We still need to lock the segment because it will modify the file
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/InstanceDataManager.java:
##########
@@ -86,11 +86,17 @@ void addRealtimeSegment(String realtimeTableName, String
segmentName)
throws Exception;
/**
- * Removes a segment from a table.
+ * Removes a segment from a table but not dropping its data from server.
*/
void removeSegment(String tableNameWithType, String segmentName)
throws Exception;
+ /**
+ * Delete segment data from the server.
+ */
+ void dropSegment(String tableNameWithType, String segmentName)
Review Comment:
Consider renaming to `deleteSegmentData` to be more specific
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##########
@@ -226,6 +230,45 @@ public void removeSegment(String tableNameWithType, String
segmentName) {
});
}
+ @Override
+ public void dropSegment(String tableNameWithType, String segmentName)
+ throws Exception {
+ // This method might modify the file on disk. Use segment lock to prevent
race condition
+ Lock segmentLock = SegmentLocks.getSegmentLock(tableNameWithType,
segmentName);
+ try {
+ segmentLock.lock();
+
+ // Clean up the segment data on default tier unconditionally.
+ File segmentDir = getSegmentDataDirectory(tableNameWithType,
segmentName);
+ if (segmentDir.exists()) {
+ FileUtils.deleteQuietly(segmentDir);
+ LOGGER.info("Deleted segment directory {} on default tier",
segmentDir);
+ }
+ // Note that this method usually happens after removeSegment, which
removes the segment object from the
+ // tableDataManager object already, so we can't check segment info from
there. In addition, tableDataManager
+ // object itself might not be present for the given table on the server
either. So the locations of segment data
+ // are derived from configs and cached info at best effort.
+ TableConfig tableConfig =
ZKMetadataProvider.getTableConfig(_propertyStore, tableNameWithType);
Review Comment:
This won't work when the table is deleted. We should be able to get the tier
from the local tier file, and use it to find the data directory
--
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]
