>From Savyasach Reddy <[email protected]>:

Attention is currently required from: Murtadha Hubail, Ali Alsuliman, Ritik Raj.
Savyasach Reddy has posted comments on this change. ( 
https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187 )

Change subject: [ASTERIXDB-3564][STO]: Avoid halts on IO operation failures
......................................................................


Patch Set 25:

(12 comments)

Commit Message:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/b8d5fad1_baff8582
PS23, Line 9: yes
> this change is internal and not user facing. […]
Done


File 
asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/HaltCallback.java:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/816cd7de_f7aba8f1
PS23, Line 44: haltOnFailure(operation)
> you should just replace all of this by operation#shouldHaltOnFailure() and 
> remove haltOnFailure in t […]
Done


File 
asterixdb/asterix-common/src/main/java/org/apache/asterix/common/ioopcallbacks/LSMIOOperationCallback.java:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/521bd352_6b6d4cd5
PS23, Line 151: getOperationMaskFilePath
> I'm wondering if we need to halt for this or just let it be there until the 
> next clean up on start u […]
Done


https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/2274137e_91826f5d
PS23, Line 155: ((MergeOperation) operation)
> mergeOperation
Done


File 
hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexBulkLoadOperatorNodePushable.java:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/18b9d5e5_6c5dab8b
PS23, Line 162:  HyracksDataException failure = null;
              :         for (IIndexBulkLoader bulkLoader : bulkLoaders) {
              :             // Let all bulk loaders end
              :             try {
              :                 // bulkloader can be null if an exception is 
thrown before it is initialized.
              :                 if (bulkLoader != null) {
              :                     bulkLoader.end();
              :                 }
              :             } catch (HyracksDataException e) {
              :                 failure = e;
              :             }
              :         }
              :         if (failure != null) {
              :             throw failure;
              :         }
> should we not abort the rest of the bulkloaders when the first bulkloader 
> fails? otherwise the other […]
Done


File 
hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIOOperation.java:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/4c893cb7_60d4b737
PS23, Line 183: shouldHalt
> rename it to shouldHaltOnFailure and add java docs
Done


File 
hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMIOOperationCallback.java:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/8082c8d0_ac5b3611
PS23, Line 61: afterFailure
> Add java docs
Done


File 
hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMTreeIndexAccessor.java:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/a7581e19_74f16bc8
PS23, Line 130: policy.retry
> Do we log anything about the re-try attempts (e.g. […]
Done


https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/34dabd54_e8fae0e5
PS23, Line 131: operation
> are you purposefully not resetting the failure to collect the failure of all 
> attempts?
Done


https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/70541edb_9676bdb6
PS23, Line 134: break;
> what happens here?
Done


File 
hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/MergeOperation.java:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/d5555646_560651af
PS23, Line 38: shouldHalt
> make this volatile
Done


File 
hyracks-fullstack/hyracks/hyracks-storage-common/src/main/java/org/apache/hyracks/storage/common/buffercache/BufferCache.java:

https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187/comment/585f61e0_d759bb1c
PS23, Line 502: c != null
> In which case c is null?
When a large page is confiscated, pages are merged, and some are marked as 
null. I've added a sanity check to see if the sum of buffers of all pages is 
matching the expected total size of buffercache.



--
To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/19187
To unsubscribe, or for help writing mail filters, visit 
https://asterix-gerrit.ics.uci.edu/settings

Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Change-Id: If253ea03f5baecbab226e527abb4267670a4233e
Gerrit-Change-Number: 19187
Gerrit-PatchSet: 25
Gerrit-Owner: Savyasach Reddy <[email protected]>
Gerrit-Reviewer: Ali Alsuliman <[email protected]>
Gerrit-Reviewer: Anon. E. Moose #1000171
Gerrit-Reviewer: Jenkins <[email protected]>
Gerrit-Reviewer: Ritik Raj <[email protected]>
Gerrit-CC: Murtadha Hubail <[email protected]>
Gerrit-Attention: Murtadha Hubail <[email protected]>
Gerrit-Attention: Ali Alsuliman <[email protected]>
Gerrit-Attention: Ritik Raj <[email protected]>
Gerrit-Comment-Date: Fri, 21 Feb 2025 18:01:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Murtadha Hubail <[email protected]>
Comment-In-Reply-To: Ritik Raj <[email protected]>
Gerrit-MessageType: comment

Reply via email to