abdullah alamoudi 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 Done Line 221: > public methods up Done Line 223: public void setLastComponentId(long componentId) throws HyracksDataException { > synchronized Done 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 Done 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: M > final Done PS17, Line 61: ongoingFlushes > scheduledFlushes Done PS17, Line 211: getOngoingFlushes > getScheduledFlushes Done PS17, Line 211: void > Return a list or unmodifiable list. Done 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 Done 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 Done PS17, Line 147: mostRecent > getMostRecentComponentId Done 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) Moved to StoragePathUtil 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? Done 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 Done 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. Done PS17, Line 62: 0 > Make this > IndexCheckpointManager.BULKLOAD_LSN Done PS17, Line 64: else > Let's check for IndexCheckpointManager.BULKLOAD_LSN Done PS17, Line 82: if (latest.getValidComponentTimestamp() == null : || componentEndTime.compareTo(latest.getValidComponentTimestamp()) > 0) { : indexCheckpointManager.setValidComponentTimestamp(componentEndTime); : } > Let's move this logic to indexCheckpointManager.setValidComponentTimestamp Done 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 s There is a check there and it throws an exception if this is violated. I think there is a chance that we schedule this job for closed datasets and didn't want that to fail. At least, not with this change. Added a comment/issue to remove this check and let it fail later. The alternative is to have another call flushIfOpen in DatasetLifeCycleManager. 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? This happens when the directory itself doesn't exist, if the directory exists, then you get back a 0 length array. PS17, Line 444: return; > Remove Done 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 Done 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 Done 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 Done PS17, Line 144: InvokeUtil.doUninterruptibly(() -> { : synchronized (logRecord) { : while (!logRecord.isFlushed()) { : logRecord.wait(); : } : } : }); > refactor with the duplicate code in appendToLogTail and LogManagerWithRepli Done 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? No reason other than that this is going to be mostly a single value and so, there is no reason to allocate an array underneath. -- 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 <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
