>From Hussain Towaileb <[email protected]>: Attention is currently required from: Peeyush Gupta.
Hussain Towaileb has posted comments on this change by Hussain Towaileb. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20743?usp=email ) Change subject: [ASTERIXDB-3634][EXT]: Add support to iceberg catalogs ...................................................................... Patch Set 11: Code-Review+1 (5 comments) File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/awsclient/EnsureCloseAWSClientFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20743/comment/f1dba7fc_e178f4e4?usp=email : PS7, Line 39: private static final long serialVersionUID = 1L; : : private transient Map<String, String> properties; : private transient String instanceId; : : private transient volatile CloseableAwsClients s3Clients; : private transient volatile CloseableAwsClients glueClients; : : private transient volatile boolean closed; : private transient volatile boolean registered; > You have used serialVersionUID and transient variables, do you need to extend > Serializable interface […] Done File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/aws/AwsUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20743/comment/6fedd6de_85e93944?usp=email : PS7, Line 199: appCtx != null > Is this for extra precaution or there particular situations appCtx can be > null here? Also, if it is […] This is for when the call is coming from the iceberg catalog to build the client. We don't have a refresh to the appCtx there, only the properties, so it will be null and we'll use the default values. File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/aws/iceberg/glue/GlueUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20743/comment/1e2176af_720591b7?usp=email : PS7, Line 106: "glue" > Make this a constant. Done File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/google/GCSUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20743/comment/54f6fc23_d9f839cc?usp=email : PS7, Line 202: int duration = 900; : if (appCtx != null) { > the default duration should be set properly here. […] Done File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/iceberg/nessie/NessieUtils.java: PS7: > This file is quite similar to RestUtils, maybe the redundant pieces of code > can be reduced. […] Agreed, however, I let them be like this for now just for isolation reasons per catalog, despite commonalities. I could probably do some sharing between them in follow up patches. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20743?usp=email To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I94f00d4d7c7d130054337696286e921e940fc6bc Gerrit-Change-Number: 20743 Gerrit-PatchSet: 11 Gerrit-Owner: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-CC: Peeyush Gupta <[email protected]> Gerrit-Attention: Peeyush Gupta <[email protected]> Gerrit-Comment-Date: Sun, 11 Jan 2026 13:23:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Peeyush Gupta <[email protected]>
