Murtadha Hubail has posted comments on this change. Change subject: [NO ISSUE][STO] Misc Storage Fixes and Improvements ......................................................................
Patch Set 17: (26 comments) https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/IndexCheckpointManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/IndexCheckpointManager.java: PS17, Line 122: String validComponentTimestamp; revert Line 221: public methods up Line 223: public void setLastComponentId(long componentId) throws HyracksDataException { synchronized https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/IndexCheckpointManagerProvider.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/IndexCheckpointManagerProvider.java: PS17, Line 64: public Move public method up https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/PrimaryIndexOperationTracker.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/context/PrimaryIndexOperationTracker.java: PS17, Line 61: ongoingFlushes scheduledFlushes PS17, Line 61: M final PS17, Line 211: void Return a list or unmodifiable list. PS17, Line 211: getOngoingFlushes getScheduledFlushes https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMIOOperationCallback.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMIOOperationCallback.java: PS17, Line 123: // Get component id of the last disk component : LSMComponentId mostRecentComponentId = : mostRecent(operation.getAccessor().getOpContext().getComponentsToBeMerged()); : // Update the checkpoint file : FileReference target = operation.getTarget(); : final ResourceReference ref = ResourceReference.of(target.getAbsolutePath()); : indexCheckpointManagerProvider.get(ref).setLastComponentId(mostRecentComponentId.getMaxId()); extract with proper name PS17, Line 136: FileReference target = operation.getTarget(); : Map<String, Object> map = operation.getParameters(); : final Long lsn = : operation.getIOOpertionType() == LSMIOOperationType.FLUSH ? (Long) map.get(KEY_FLUSH_LOG_LSN) : 0L; : final LSMComponentId id = (LSMComponentId) map.get(KEY_FLUSHED_COMPONENT_ID); : final ResourceReference ref = ResourceReference.of(target.getAbsolutePath()); : final String componentEndTime = AbstractLSMIndexFileManager.getComponentEndTime(ref.getName()); : indexCheckpointManagerProvider.get(ref).flushed(componentEndTime, lsn, id.getMaxId()) extract with proper name PS17, Line 147: mostRecent getMostRecentComponentId https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/storage/IIndexCheckpointManagerProvider.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/storage/IIndexCheckpointManagerProvider.java: PS17, Line 43: /** : * Get the path of the index containing the passed reference : * : * @param ref : * @return : * @throws HyracksDataException : */ : Path getIndexPath(ResourceReference ref) throws HyracksDataException; Move this to a better place (e.g. PersistentLocalResourceRepository) https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/storage/IndexCheckpoint.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/storage/IndexCheckpoint.java: PS17, Line 36: EMPTY_INDEX_LAST_COMPONENT_ID This doesn't belong here. Maybe move it to LSMComponentId? https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogType.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/transactions/LogType.java: PS17, Line 41: WAIT Fix this https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-replication/src/main/java/org/apache/asterix/replication/messaging/MarkComponentValidTask.java File asterixdb/asterix-replication/src/main/java/org/apache/asterix/replication/messaging/MarkComponentValidTask.java: PS17, Line 47: masterLsn < A better way to do this is to define a constant called MERGE_LSN. Either remove the comment or link to LSMIOOperationCallback.INVALID_LSN. PS17, Line 62: 0 Make this > IndexCheckpointManager.BULKLOAD_LSN PS17, Line 64: else Let's check for IndexCheckpointManager.BULKLOAD_LSN PS17, Line 82: if (latest.getValidComponentTimestamp() == null : || componentEndTime.compareTo(latest.getValidComponentTimestamp()) > 0) { : indexCheckpointManager.setValidComponentTimestamp(componentEndTime); : } Let's move this logic to indexCheckpointManager.setValidComponentTimestamp and rename it to something indicating (update if newer) https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/std/FlushDatasetOperatorDescriptor.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/std/FlushDatasetOperatorDescriptor.java: PS17, Line 86: DatasetInfo datasetInfo = datasetLifeCycleManager.getDatasetInfo(datasetId.getId()); : synchronized (datasetLifeCycleManager) { : if (datasetInfo.isOpen()) { do this inside datasetLifeCycleManager.flushDataset. It is not fun to do synchronized (datasetLifeCycleManager) here. https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/resource/PersistentLocalResourceRepository.java: PS17, Line 434: if (indexComponentFiles == null) { : throw new IOException(index + " doesn't exist or an IO error occurred"); : } why is this valid? can't we have an index without any components? PS17, Line 444: return; Remove https://asterix-gerrit.ics.uci.edu/#/c/2632/17/asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogBuffer.java File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/logging/LogBuffer.java: PS17, Line 92: logRecord.getLogSource() == LogSource.LOCAL && logRecord.getLogType() != LogType.FLUSH : && logRecord.getLogType() != LogType.WAIT && logRecord.getLogType() != LogType.WAIT_FOR_FLUSHES extract static method with proper name PS17, Line 103: logRecord.getLogType() == LogType.JOB_COMMIT || logRecord.getLogType() == LogType.ABORT : || logRecord.getLogType() == LogType.WAIT : || logRecord.getLogType() == LogType.WAIT_FOR_FLUSHES extract static method with proper name https://asterix-gerrit.ics.uci.edu/#/c/2632/17/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: PS17, Line 138: logRecord.isFlushed(false); : flushLogsQ.add(logRecord); : return; : } else if (logRecord.getLogType() == LogType.WAIT_FOR_FLUSHES) { : logRecord.isFlushed(false); : flushLogsQ.add(logRecord); refactor and decide on to wait or not based on the log type PS17, Line 144: InvokeUtil.doUninterruptibly(() -> { : synchronized (logRecord) { : while (!logRecord.isFlushed()) { : logRecord.wait(); : } : } : }); refactor with the duplicate code in appendToLogTail and LogManagerWithReplication#log https://asterix-gerrit.ics.uci.edu/#/c/2632/17/hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractIoOperation.java File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/AbstractIoOperation.java: PS17, Line 144: LinkedList why linked list? -- To view, visit https://asterix-gerrit.ics.uci.edu/2632 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: If24c9baaac2b79e7d1acf47fa2601767388ce988 Gerrit-PatchSet: 17 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <mhub...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
