>From Peeyush Gupta <[email protected]>: Attention is currently required from: Hussain Towaileb.
Peeyush Gupta has posted comments on this change by Hussain Towaileb. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478?usp=email ) Change subject: [ASTERIXDB-3634][EXT]: Add support to Iceberg pt.2 ...................................................................... Patch Set 5: (9 comments) File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/awsclient/MyCustomAwsClientFactory.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/0a08d553_3a866d25?usp=email : PS5, Line 42: AwsClientFactory There are already classes like org.apache.iceberg.aws.AssumeRoleAwsClientFactory, can't we use those? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/4efe4ad4_1a0ce70d?usp=email : PS5, Line 42: MyCustomAwsClientFactory The name could be EnsureCloseAWSClientFactory or something related. https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/87858b73_5dbac948?usp=email : PS5, Line 69: // if (proxyGlueClient == null) { : // GlueClient delegate = (GlueClient) glueClients.getConsumingClient(); : // proxyGlueClient = (GlueClient) Proxy.newProxyInstance( : // GlueClient.class.getClassLoader(), : // new Class<?>[]{GlueClient.class}, : // new GlueClientInvocationHandler(delegate)); : // } : // return proxyGlueClient; remove commented out code https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/144186b5_7e7367b5?usp=email : PS5, Line 79: @Override : public KmsClient kms() { : return null; : } : : @Override : public software.amazon.awssdk.services.dynamodb.DynamoDbClient dynamo() { : return null; : } What are these functions for? Is returning null here ok? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/9aa8ed06_6e362f3a?usp=email : PS5, Line 112: private class GlueClientInvocationHandler implements InvocationHandler { : private final GlueClient delegate; : private volatile boolean closed = false; : : GlueClientInvocationHandler(GlueClient delegate) { : this.delegate = delegate; : } : : @Override : public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { : String name = method.getName(); : if ("close".equals(name)) { : if (!closed) { : closed = true; : try { : delegate.close(); : } finally { : // Close both sets; no-op if not built or already closed : AwsUtils.closeClients(glueClients); : AwsUtils.closeClients(s3Clients); : } : } : return null; : } : return method.invoke(delegate, args); : } : } This seems to be unused. Are you planning to use it in subsequent patches? File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/aws/delta/IcebergFileRecordReader.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/312c2839_efdf8a6f?usp=email : PS5, Line 19: delta I think this should be in the iceberg aws.iceberg package https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/33525723_92f551cf?usp=email : PS5, Line 146: } do we need to close the catalog here? File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/aws/delta/converter/IcebergConverterContext.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/6ab6cf03_b0919cc5?usp=email : PS5, Line 19: aws.delta.converter I think this should be in the iceberg aws.iceberg.converter package or just aws.iceberg package https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478/comment/60216dec_c9539819?usp=email : PS5, Line 57: DeltaOptions.DECIMAL_TO_DOUBLE, It will be better to create a separate IcebergOptions class and then using that class here. We should Ideally have a CommonOptions class and use that to avoid rewriting the exact same option multiple times, but that is not related to this patch. -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/20478?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: I531c613fe061367b2cb5c187bc985b39d6de17d1 Gerrit-Change-Number: 20478 Gerrit-PatchSet: 5 Gerrit-Owner: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Hussain Towaileb <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Peeyush Gupta <[email protected]> Gerrit-Attention: Hussain Towaileb <[email protected]> Gerrit-Comment-Date: Mon, 08 Dec 2025 19:35:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
