>From Peeyush Gupta <[email protected]>: Attention is currently required from: Murtadha Al Hubail, Ali Alsuliman. Peeyush Gupta has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598 )
Change subject: [NO ISSUE][TX]: Statement level atomicity for inserts/upserts/deletes ...................................................................... Patch Set 4: (19 comments) File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/GlobalTxManager.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/dbe3f3dc_ff6cde2e PS3, Line 56: synchronized > why do you need to synchronize on the manager itself here? removed synchronization. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/2f52ef1b_2b0925fd PS3, Line 66: txnContextRepository.get(jobId); : if (context == null) { : throw new ACIDException( : "Error committing atomic transaction, transaction for jobId " + jobId + " does not exist"); : } > you can refactor all of these as getJobTx() Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/e1f4c186_efe087d9 PS3, Line 74: InterruptedException e > You should almost always re-interrupt the thread (Thread.currentThread. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/583332d8_3ff397f7 PS3, Line 87: synchronized > why do you need to synchronize on the manager itself here? removed synchronization. File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/AtomicJobCommitMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/e0a7600f_06c22cbe PS3, Line 65: throw new RuntimeException(e); > Not sure if we want to throw a RuntimeException here but you can add a TODO > related to GlobalTx fail […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/f258036e_dc5b0aba PS3, Line 69: @Override : public boolean isWhispered() { : return INcAddressedMessage.super.isWhispered(); : } > remove this Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/AtomicJobRollbackMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/688b8bc3_e288e956 PS3, Line 84: @Override : public boolean isWhispered() { : return INcAddressedMessage.super.isWhispered(); : } > remove Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/EnableMergeMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/310257da_c049ecc7 PS3, Line 39: datasetId > we will probably need more than 1 datasetId if we use the same path for > atomic metadata tx that invo […] Ack https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/a995cf67_a40a6cc3 PS3, Line 52: @Override : public boolean isWhispered() { : return INcAddressedMessage.super.isWhispered(); : } > remove Done File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/13e671b6_935b79c7 PS3, Line 3488: isAtomic > isBoolean is typically used for the getter of a boolean and not the boolean > itself. […] Done https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/b3b8090b_21770185 PS3, Line 3511: JobUtils.runJob > if you run the job before beginning the tx, how do ensure that no messages > are missed if a node is f […] You are right, this can issue you mentioned. I need the job id to begin the transaction, but the job id is returned once the job is started. Any pointers to get the job id without starting the job, or should I start the job inside the begin transaction? File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/utils/StorageConstants.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/19572203_66cec41a PS4, Line 34: "/tmp/"; Any suggestions to how to set the CC storage root directory? Should we get it from ns server? File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryInsertOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/7c12ab27_f38e754e PS4, Line 226: if (((ILSMIndex) indexes[i]).isAtomic()) { : ((PrimaryIndexOperationTracker) ((ILSMIndex) indexes[i]).getOperationTracker()).clear(); : } I had to add this, because in case of a failure before committing the previous statement, we need to clear up the memory components and temporary disk components. File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/ae644f02_38c223af PS3, Line 567: private void commitAt > try to refactor some of the code with other operators I have added a TODO for this, I will refactor this in the next patch for sure. File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/AtomicTransactionLog.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/f9a60cb6_594f6908 PS3, Line 41: @JsonCreator > Those @Json annotations start to get complicated and not maintainable once > too many of them are used […] Ack File asterixdb/asterix-transactions/src/main/java/org/apache/asterix/transaction/management/service/transaction/GlobalTransactionContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/6fe862c3_24fea81a PS3, Line 120: @Override : public void setTimeout(boolean isTimeout) { : this.isTimeout = isTimeout; : } : : @Override : public boolean isTimeout() { : return isTimeout; : } > are these ever used? removed. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/091543d8_5b16f00a PS3, Line 147: catch (HyracksDataException e) { : throw new RuntimeException(e); : } catch (JsonProcessingException e) { : throw new RuntimeException(e); : } catch (ClosedByInterruptException e) { : throw new RuntimeException(e); : } > just catch Exception or (HyracksDataException | JsonProcessingException | ... > e). […] Ack File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/api/ILSMComponentId.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/2049db17_57e1cb8c PS3, Line 31: @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.WRAPPER_OBJECT, property = "type") : @JsonSubTypes({ @JsonSubTypes.Type(value = LSMComponentId.class, name = "lsmComponentId"), }) > You can see how complicated those annotations get even in this patch, not to > mention their readabili […] Ack File hyracks-fullstack/hyracks/hyracks-storage-am-lsm-common/src/main/java/org/apache/hyracks/storage/am/lsm/common/impls/LSMComponentId.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/8c9f9919_deda2d37 PS3, Line 29: Serializable > The less Serializable stuff we have, the better. […] Ack -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598 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: I3b4aefeba07be921d128255393aec1b703198a40 Gerrit-Change-Number: 17598 Gerrit-PatchSet: 4 Gerrit-Owner: Peeyush Gupta <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Al Hubail <[email protected]> Gerrit-Attention: Murtadha Al Hubail <[email protected]> Gerrit-Attention: Ali Alsuliman <[email protected]> Gerrit-Comment-Date: Wed, 28 Jun 2023 21:59:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Murtadha Al Hubail <[email protected]> Gerrit-MessageType: comment
