Core: Close the MetricsReporter when the Catalog is closed. #9353
Conversation
|
cc @nastra |
| @@ -487,6 +486,7 @@ public Configuration getConf() { | |||
|
|
|||
| @Override | |||
| public void close() throws IOException { | |||
| super.close(); | |||
There was a problem hiding this comment.
Is not using closeableGroup a deliberate choice?
There was a problem hiding this comment.
I don’t quite understand what problem can be solved by using closeableGroup it's here, Can you give some examples?
There was a problem hiding this comment.
I think in order to close metric reporter when cloase the catalog, it can also be implemented to add super.metricsReporter() to closeableGroup on line 143, but this require to change the BaseMetastoreCatalog.metricsReporter() method from private to protected.
This approach keep the Closeable in concrete class instead of abstract base class, and it seem to be more aligned with how other closable are handled in the this class.
There was a problem hiding this comment.
I think in order to close metric reporter when cloase the catalog, it can also be implemented to add
super.metricsReporter()to closeableGroup on line 143, but this require to change theBaseMetastoreCatalog.metricsReporter()method from private to protected.This approach keep the
Closeablein concrete class instead of abstract base class, and it seem to be more aligned with how other closable are handled in the this class.
I agree with this statement. In the Java language, it is generally a common practice to call super.close() when a subclass overrides the method of the parent class, so I did do this at the beginning. If want to unify for the closable approach, it is indeed better to use closeableGroup. I will modify this part of the code.
There was a problem hiding this comment.
@dramaticlly Can you help me take a look?
There was a problem hiding this comment.
curious, is there any motivation to keep Closable in abstract class BaseMetastoreCatalog?
There was a problem hiding this comment.
look here:
Because of this, I think BaseMetastoreCatalog should keep Closeable to limit the behavior of subclasses, because the metricsReporter in BaseMetastoreCatalog must be released when the catalog is closed.
There was a problem hiding this 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.
There was a problem hiding this comment.
This looks good to me, although I do think there are some possible improvements we could do for having MetricsReporting being a separate mixin for catalogs, but those are out of scope for what this PR is doing.
I'll wait for a bit in case @aokolnychyi has any further comments before merging.
| @@ -143,6 +142,7 @@ void initialize( | |||
| this.closeableGroup = new CloseableGroup(); | |||
| closeableGroup.addCloseable(dynamo); | |||
| closeableGroup.addCloseable(fileIO); | |||
| closeableGroup.addCloseable(metricsReporter()); | |||
There was a problem hiding this comment.
I just wanted to point out that this effectively will initialize the reporter much earlier (during catalog instantiation) than previously (during table loading). This is probably ok
link #9349