[GitHub] [cassandra] yifan-c commented on a diff in pull request #2251: CASSANDRA-18373 Node Draining Aborts All Current SSTables Imports

["yifan-c (via GitHub)" <gi...@apache.org>]

yifan-c commented on code in PR #2251:
URL: https://github.com/apache/cassandra/pull/2251#discussion_r1156294284


##########
src/java/org/apache/cassandra/db/SSTableImporter.java:
##########
@@ -130,6 +131,8 @@ synchronized List<String> importNewSSTables(Options options)
             {
                 try
                 {
+                    if (abort)
+                        throw new InterruptedException("SSTables import has been aborted");

Review Comment:
   If the only reason for aborting is because of draining. Would using `operationMode` simplify the implementation? The changes in CFS and StorageService can be dropped. 
   
   ```
   if (StorageService.instance.getOperationMode().equalsIgnoreCase(StorageService.Mode.DRAINING.toString()))
   ``` 
   
   Another question is that is it the only place to abort the import? The sstables could be opening already before invoking `drain`. Those sstables won't be aborted. 
   It is not possible/clean to add the check everywhere. It might make sense to add one extra check before line#183 to avoid loading and building SI of the newly opened sstables. 



-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org