Young-Seok Kim has posted comments on this change. Change subject: ASTERIXDB-1425 & ASTERIXDB-1450: Fix LogReader random reads ......................................................................
Patch Set 4: (5 comments) https://asterix-gerrit.ics.uci.edu/#/c/867/4/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: Line 370: continue; Do we delete from the oldest-log file to newer ones? I think we should do that. Also, we should not leave any hole between log files. For example, suppose that there are f1, f2, and f3 log files. f1 is read by an abort thread. If the checkpoint thread tries to deletes f1 and f2 since checkpointLSN belongs to f3, then the checkpoint thread will skip f1 but f2 will be deleted. So, abort thread may not find f2 if the abort job's log records span f1 and f2 files. Do we handle this case properly? https://asterix-gerrit.ics.uci.edu/#/c/867/4/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogReader.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogReader.java: Line 226: while (readLSN > flushLSN.get()) { nice catch! Line 254: If I remember correctly, we separated two cases: rollback and recovery. Recovery uses next() and rollback uses read(). Thus, read() never see TRUNCATED status and why we return exception. So, this removed code seems OK, but the TRUNCATED case should return the LSN is out of bounds exception or log is truncated exception. Ian may know about this. @Ian Do you have any idea regarding this code? Line 267: //now read the complete log record This will cause infinite loop in the current while loop since RecordReadStatus status = logRecord.readLogRecord(readBuffer); ==> This will always return TRUNCATED status and read again and again. https://asterix-gerrit.ics.uci.edu/#/c/867/4/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/recovery/RecoveryManager.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/recovery/RecoveryManager.java: Line 708: if (firstLSN == TransactionManagementConstants.LogManagerConstants.TERMINAL_LSN || firstLSN >= lastLSN) { must be "firstLSN > lastLSN", not firstLSN >= lastLSN. if they are the same, then there is a last log record which might be used to abort. -- To view, visit https://asterix-gerrit.ics.uci.edu/867 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1c75ca4a7c8fe197451126392389d4baecbd7e45 Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Young-Seok Kim <kiss...@gmail.com> Gerrit-HasComments: Yes
