Ian Maxon has posted comments on this change. Change subject: [ASTERIXDB-1708] Prevent log deletion during scan ......................................................................
Patch Set 5: (9 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. #replayReplic Good point, I hadn't considered that method, I was under the impression the only other readers besides rollback would all be during offline recovery. 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 Done Line 193: * with other concurrent readers of the log that request > WS Done Line 196: > this test case is too complicated. I'm not entirely sure I understand the t The test is just reproducing the bug, which is doing rollback, and then having the log deleted after the minimum LSN is determined, but before the reader has been opened. PS5, Line 210: System.out.println("ckpoint: " + lastCkpoint); > Use logger Whoops, removed, that's just debug code. 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 a If you had two readers that locked two different LSNs, and it was just one lock, how would each reader know when it was safe to unblock the checkpoint? Locking the specific LSNs and keeping track of how many are at each LSN makes it unambiguous. 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? Cause, then it will be sorted on the key, so hopefully less of the keys will have to be examined to determine if the checkpoint can proceed. Probably not very likely to have any effect though. 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 sho I suppose it could synchronize on something else, perhaps the recovery manager. The requirement is just that determining the minimum LSN and acquiring or examining the lock on that LSN has to be synchronous. PS5, Line 108: return; > if this happens, then something is wrong Yes. I suppose an exception could be thrown? -- 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
