>From Murtadha Al Hubail <[email protected]>: Attention is currently required from: Peeyush Gupta. Murtadha Al Hubail has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598 )
Change subject: WIP: Statement level atomicity for inserts/upserts/deletes ...................................................................... Patch Set 3: (17 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/30f3eaf1_b2c47f21 PS3, Line 56: synchronized why do you need to synchronize on the manager itself here? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/4be4c18d_44d99679 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() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/9068e724_9f4d9403 PS3, Line 74: InterruptedException e You should almost always re-interrupt the thread (Thread.currentThread.interrupt) after catching InterruptedException. We will probably need another patch just to deal with exceptions throw from this class. I believe some of them currently will result in shutting down the CC when a RuntimeException is thrown and that might not be what we want. However, we can refine these in subsequent patches. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/d9967803_af479b71 PS3, Line 87: synchronized why do you need to synchronize on the manager itself here? File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/AtomicJobCommitMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/b51130ff_3c84b1b3 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 failure handling https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/ab48de53_947c1c3b PS3, Line 69: @Override : public boolean isWhispered() { : return INcAddressedMessage.super.isWhispered(); : } remove this File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/AtomicJobRollbackMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/59a222a0_72376b3b PS3, Line 84: @Override : public boolean isWhispered() { : return INcAddressedMessage.super.isWhispered(); : } remove File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/message/EnableMergeMessage.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/ad40ffdf_7bbe4a2e PS3, Line 39: datasetId we will probably need more than 1 datasetId if we use the same path for atomic metadata tx that involve multiple datasets. We can keep it like this and update if needed https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/afbd8ee9_029db96d PS3, Line 52: @Override : public boolean isWhispered() { : return INcAddressedMessage.super.isWhispered(); : } remove File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/6122a433_9d0b2179 PS3, Line 3488: isAtomic isBoolean is typically used for the getter of a boolean and not the boolean itself. I know we don't have one convention in the codebase, but it might not be too late to stick with one. So, I'd use "atomic" here and same for other booleans. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/5492a51b_93c43770 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 fast enough and sends a message before that? File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/LSMPrimaryUpsertOperatorNodePushable.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/efc8871c_b2062f61 PS3, Line 567: private void commitAt try to refactor some of the code with other operators 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/1f698e53_396eb483 PS3, Line 41: @JsonCreator Those @Json annotations start to get complicated and not maintainable once too many of them are used, specially nested ones. For more complicated classes, I think it is better to do the json serialization/deserialization manually (potentially with some versioning) and avoid the magic those annotations do. You can leave it like this for now and let's discuss it after. 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/4154fb7f_87c2372f PS3, Line 120: @Override : public void setTimeout(boolean isTimeout) { : this.isTimeout = isTimeout; : } : : @Override : public boolean isTimeout() { : return isTimeout; : } are these ever used? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/17598/comment/ca9527e6_2aaf67c6 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). Same for all places, but as I said, we will need to review these 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/778ba77a_878b6e06 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 readability after a while. 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/c70afd7e_0f634205 PS3, Line 29: Serializable The less Serializable stuff we have, the better. You can leave it like this for now but let's discuss if we can just use json for the message that needs to send this info and maybe just send the maxId. -- 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: 3 Gerrit-Owner: Peeyush Gupta <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-CC: Ali Alsuliman <[email protected]> Gerrit-CC: Murtadha Al Hubail <[email protected]> Gerrit-Attention: Peeyush Gupta <[email protected]> Gerrit-Comment-Date: Tue, 27 Jun 2023 02:58:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
