Ian Maxon has posted comments on this change. Change subject: [ASTERIXDB-1708] Prevent log deletion during scan ......................................................................
Patch Set 9: (11 comments) https://asterix-gerrit.ics.uci.edu/#/c/2256/9/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: Line 511: public void replayReplicaPartitionLogs(Set<Integer> partitions, boolean flush) throws HyracksDataException { > synchronized Done PS9, Line 590: if (localMinFirstLSN > firstLSN) { : firstLSN = localMinFirstLSN; : } > what is the reason behind this change? I just ended up rewriting that part, Could revert it. https://asterix-gerrit.ics.uci.edu/#/c/2256/9/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: PS9, Line 74: CheckpointingTest > Here is a simpler test if you would like. You just need to make MetadataTxn The test I have here is just trying to replicate the behavior in the bug closely by having the checkpoint happen as an abort is going on. https://asterix-gerrit.ics.uci.edu/#/c/2256/9/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: Line 49: > /** Done Line 51: > /** Done https://asterix-gerrit.ics.uci.edu/#/c/2256/9/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: PS9, Line 79: txnLogFileId2ReaderCount > You need to remove this and all of its usage from the class Done https://asterix-gerrit.ics.uci.edu/#/c/2256/9/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: PS9, Line 66: protected Map<TxnId, Long> securedLSNs; > move it to CheckpointManager and make it private final Done https://asterix-gerrit.ics.uci.edu/#/c/2256/9/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: PS9, Line 76: for (Long l : securedLSNs.values()) { : if (minFirstLSN >= l) { : return minFirstLSN; : } : } > Refactor as getMinSecuredLsn and please use a better variable name than I. Done PS9, Line 93: throws IllegalStateException > don't put runtime exceptions in throws Done PS9, Line 94: if (securedLSNs.containsKey(id)) { : securedLSNs.remove(id); : } else { : throw new IllegalStateException( : "Attempt made to complete a " + "transaction which was not previously secured"); : } > It is fine to do securedLSNs.remove(id); only here. The API doesn't say you Sure, I was just carrying over on the analagous comment to unlockLSN() before the signature was changed. https://asterix-gerrit.ics.uci.edu/#/c/2256/9/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/TransactionManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/TransactionManager.java: PS9, Line 111: ACIDException | > remove Done -- 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: 9 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
