amogh-jahagirdar commented on code in PR #9353:
URL: https://github.com/apache/iceberg/pull/9353#discussion_r1454385050
##########
aws/src/main/java/org/apache/iceberg/aws/dynamodb/DynamoDbCatalog.java:
##########
@@ -487,6 +486,7 @@ public Configuration getConf() {
@Override
public void close() throws IOException {
+ super.close();
Review Comment:
I think that's reasonable. In my mind it would have been ideal if metrics
reporting was a separate Mixin interface and each concrete implementation
implements that mixin ("SupportsMetricsReporting") and takes responsibility for
providing a metrics reporter implementation and overriding close. The tradeoff
is there is more close implementations but I think it's generally better to
have those implementations be at the concrete level. That's a wider change, and
probably I'm missing an implication of that design so the current approach is
fine to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
