Murtadha Hubail has posted comments on this change. Change subject: [ASTERIXDB-1708] Prevent log deletion during scan ......................................................................
Patch Set 5: (11 comments) https://asterix-gerrit.ics.uci.edu/#/c/2256/5/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/RecoveryManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/RecoveryManager.java: PS5, Line 582: try { : synchronized (logMgr) { : long localMinFirstLSN = getLocalMinFirstLSN(); : firstLSN = Math.max(firstLSN, localMinFirstLSN); : checkpointManager.lockLSN(firstLSN); : } : } catch (HyracksDataException e) { : checkpointManager.unlockLSN(firstLSN); : throw new ACIDException(e); : } if you do this here in the rollback call, other readers (e.g. #replayReplicaPartitionLogs) will not be protected. https://asterix-gerrit.ics.uci.edu/#/c/2256/5/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/logging/CheckpointingTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/logging/CheckpointingTest.java: Line 192: * job to ensure that the checkpointing coexists peacefully WS Line 193: * with other concurrent readers of the log that request WS Line 196: this test case is too complicated. I'm not entirely sure I understand the test scenario. The test should be much simpler than this. All what you need is multiple txn log files with a secured lsn in the first one, then try to checkpoint and the checkpoint should fail to delete the log file with the secured lsn. PS5, Line 210: System.out.println("ckpoint: " + lastCkpoint); Use logger https://asterix-gerrit.ics.uci.edu/#/c/2256/5/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ICheckpointManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/ICheckpointManager.java: PS5, Line 50: void unlockLSN(long lsn); : : void lockLSN(long lsn); why lock specific LSNs? all what you need is to block the checkpoint from advancing the current low water mark until the log reader is done. https://asterix-gerrit.ics.uci.edu/#/c/2256/5/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogManager.java: PS5, Line 569: synchronized why? PS5, Line 599: synchronized why? https://asterix-gerrit.ics.uci.edu/#/c/2256/5/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/recovery/AbstractCheckpointManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/recovery/AbstractCheckpointManager.java: PS5, Line 92: TreeMap why TreeMap? https://asterix-gerrit.ics.uci.edu/#/c/2256/5/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/recovery/CheckpointManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/recovery/CheckpointManager.java: PS5, Line 95: synchronized (txnSubsystem.getLogManager() I don't see any reason for synchronizing on logManager. The log manager shouldn't be involved in protecting LSN. All ingestion threads are already competing on log manager, it wouldn't be wise to add additional unnecessary synchronization there. PS5, Line 108: return; if this happens, then something is wrong -- To view, visit https://asterix-gerrit.ics.uci.edu/2256 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icff1a520af24c8fac8e5836cdbf46425b78b1260 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Ian Maxon <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Luo Chen <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
