Murtadha Hubail has posted comments on this change. Change subject: ASTERIXDB-1425 & ASTERIXDB-1450: Fix LogReader random reads ......................................................................
Patch Set 4: (6 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 362: for (Long id : logFileIds) { > MAJOR SonarQube violation: Done Line 370: continue; > Do we delete from the oldest-log file to newer ones? Done 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. The API of this class should not care about rollback or recovery, and in fact, rollback uses both next and read. The condition on the deleted code tries to read again from the log file only if the LSN of the log record we are trying to read starts at the end of the currently filled buffer. According to the 4 cases right above this code, in the last case, it is possible that the LSN of the log record we are trying to read is before the last byte of the buffer, but it might not be completely there (truncated). Therefore, we will try to read it from the currently filled buffer first, and if it is truncated, we must read from the log file again. Line 267: //now read the complete log record > This will cause infinite loop in the current while loop since See above comment. This won't happen since we know the requested LSN is valid since it has already reached flushLSN. So, the log file should contain it unless the file was deleted (which should not happen now). If the log file is deleted due to a bug, an I/O exception during the read will be thrown which will break the loop. However, I added a check which validates that we could read at least something from the file. 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. Done -- 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
