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

Reply via email to