Murtadha Hubail has posted comments on this change. Change subject: ASTERIXDB-1152: Delete storage data of old instances ......................................................................
Patch Set 8: (6 comments) https://asterix-gerrit.ics.uci.edu/#/c/469/8/asterix-app/src/main/java/org/apache/asterix/api/common/AsterixAppRuntimeContext.java File asterix-app/src/main/java/org/apache/asterix/api/common/AsterixAppRuntimeContext.java: Line 131: private final boolean initialRun; > Why does "initialRun" get stored in the context? It seems that the lifecycl I implemented first by passing it to the initialize() method, then I wanted to factor out the logic to get NEW_UNIVERSE state based on the flag in this class as well as NCApplicationEntryPoint. I reverted it to be passed in the initialize(), and remove the logic the NEW_UNIVERSE logic. https://asterix-gerrit.ics.uci.edu/#/c/469/8/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java: Line 96 > This should probably be removed before committing this change. Done Line 70: * to make sure when ResourceIdFactory is initialize on this LocalResourceRepository, it gets the correct value. > s/intialize/initialized/ Removed the comment. Line 121: System.out.println("storageMetadataDir: " + storageMetadataDir.getAbsolutePath()); > ditto Removed. Line 135: if (storageRootDirName.startsWith(System.getProperty("file.separator"))) { > What's the difference between 'System.getProperty("file.separator")' and ja In our case they are the same and should work on other OSs. The only difference is that System.getProperty can be overridden by System.setProperty, whereas File.separator will always return the default OS file system separator. I prefer File.separator but keeping System.getProperty in this class might be useful in HDFS or some other file system implementation. P.S. I didn't change it to avoid getting asked this question in the code review :) https://asterix-gerrit.ics.uci.edu/#/c/469/8/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepositoryFactory.java File asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepositoryFactory.java: Line 34: this.systemState = systemState; > It seems strange to materialize the state in this factory, as the state of Good point. The only reason I did it this way because this factory is used only during the RuntimeContext initialization then it gets garbage collected. I will remove it from here and make a direct call to PersistentLocalResourceRepository from the RuntimeContext to delete the storage based on the system state there. I hate unnecessary factories :) -- To view, visit https://asterix-gerrit.ics.uci.edu/469 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibb6c6949bdf2ed6c3e491fa66a23491ff34fc830 Gerrit-PatchSet: 8 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Taewoo Kim <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Young-Seok Kim <[email protected]> Gerrit-HasComments: Yes
