[Core] Support TableScan resources clean #2123
Conversation
schnappi17
force-pushed
the
support-close-scan
branch
from
15e9988 to
d87cad8
Compare
| @@ -47,4 +47,7 @@ public interface TableScan { | |||
| interface Plan { | |||
| List<Split> splits(); | |||
| } | |||
|
|
|||
| /** Close this scan, clean resources here. */ | |||
| void close(); | |||
There was a problem hiding this comment.
TableScan is a @Public interface, do we really need to add a close() method here?
| @@ -187,6 +187,11 @@ protected StartingScanner createStartingScanner(boolean isStreaming) { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public void close() { | |||
| // do nothing yet | |||
There was a problem hiding this comment.
All the close() methods in AbstractInnerTableScan and ReadOnceTableScan are empty, why do we need it?
There was a problem hiding this comment.
All the
close()methods inAbstractInnerTableScanandReadOnceTableScanare empty, why do we need it?
@FangYongs This is a base pr, the scan metrics will rebase this pr and do substantial metrics resource clean in the close method.
There was a problem hiding this comment.
Hi @schnappi17
I think we should avoid adding close for metrics only.
- This may lead to lots of closes in our codes, looks very tricky.
- This breaks back Backwards Compatibility.
We may need to revisit Metrics design. I know Hudi also use static Metrics field to maintain all metrics, but when I take a look to its implementation. It is very tricky. It never delete metrics, and the shutdown of metrics is very casualness.
When I take a look to the metrics of Flink: https://issues.apache.org/jira/browse/FLINK-7876 It looks very good, Paimon, may not need to manage metrics ourselves, just register metrics externally through an interface.
For example, registering with the metrics system of a computing engine, or injecting users into their own metrics system.
The interface can just be a class like MetricRegistry.
|
Thanks @JingsongLi and @FangYongs for reviewing and suggestions, I'll take a look at this and thinking about the metrics interfaces design. |
Purpose
Linked issue: close #2122