Murtadha Hubail has posted comments on this change. Change subject: [ASTERIXDB-1708] Prevent log deletion during scan ......................................................................
Patch Set 9: (12 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 PS9, Line 513: TxnId randomDummyTxnId = new TxnId(ThreadLocalRandom.current().nextInt(Integer.MIN_VALUE, -1)); private static final PS9, Line 590: if (localMinFirstLSN > firstLSN) { : firstLSN = localMinFirstLSN; : } what is the reason behind this change? 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 MetadataTxnTest#addDataset public static. public class CheckpointManagerTest { protected static final String TEST_CONFIG_FILE_NAME = "src/test/resources/cc-small-txn-log-partition.conf"; private static final AsterixHyracksIntegrationUtil integrationUtil = new AsterixHyracksIntegrationUtil(); @Before public void setUp() throws Exception { System.setProperty(GlobalConfig.CONFIG_FILE_PROPERTY, TEST_CONFIG_FILE_NAME); integrationUtil.init(true, TEST_CONFIG_FILE_NAME); } @After public void tearDown() throws Exception { integrationUtil.deinit(true); } @Test public void securedLowWatermarkTest() throws Exception { ICcApplicationContext ccAppCtx = (ICcApplicationContext) integrationUtil.getClusterControllerService().getApplicationContext(); INcApplicationContext appCtx = (INcApplicationContext) integrationUtil.ncs[0].getApplicationContext(); IRecoveryManager recoveryManager = appCtx.getTransactionSubsystem().getRecoveryManager(); long securedLowWaterMark = recoveryManager.getMinFirstLSN(); // secure the current low water mark TxnId securedTxnId = new TxnId(-1); ICheckpointManager checkpointManager = appCtx.getTransactionSubsystem().getCheckpointManager(); checkpointManager.secure(securedTxnId); LogManager logManager = (LogManager) appCtx.getTransactionSubsystem().getLogManager(); Assert.assertEquals(1, logManager.getLogFileIds().size()); // insert records until we have an additional log file String srcDsName = "test_ds"; TestDataUtil.createIdOnlyDataset(srcDsName); Dataset dataset = TestDataUtil.getDataset(integrationUtil, srcDsName); int i = 0; while (logManager.getLogFileIds().size() == 1) { MetadataTxnTest.addDataset(ccAppCtx, dataset, i, false); i++; } // flush datasets to move low water mark appCtx.getDatasetLifecycleManager().flushAllDatasets(); long newLowWatermark = recoveryManager.getMinFirstLSN(); Assert.assertTrue(newLowWatermark > securedLowWaterMark); // ensure checkpoint will fail to delete the old log file long checkpointLsn = checkpointManager.tryCheckpoint(newLowWatermark); Assert.assertEquals(securedLowWaterMark, checkpointLsn); Assert.assertEquals(2, logManager.getLogFileIds().size()); // complete the secured txn and ensure checkpoint will succeed and delete the old log file checkpointManager.completed(securedTxnId); checkpointLsn = checkpointManager.tryCheckpoint(newLowWatermark); Assert.assertEquals(checkpointLsn, newLowWatermark); Assert.assertEquals(1, logManager.getLogFileIds().size()); } } 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: /** * Secures the current low-water mark until the transaction identified by {@code id} completes. * * @param id * @throws HyracksDataException */ Line 51: /** * Notifies this {@link ICheckpointManager} that the transaction identified by {@code id} completed. * * @param id */ 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 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 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. Also, make the check at the beginning of this method against checkpointTargetLSN to avoid scheduling the flushes (i.e. never move beyond the current secured low water mark). PS9, Line 93: throws IllegalStateException don't put runtime exceptions in throws 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 can only complete secured txns. 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 -- 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
