Core: Add metrics reporter for serializable table

[wangyinsheng]

The metrics reporter will be lost when a table be sent to other nodes in a cluster through serializable table, This mr rebuild the metrics reporter when the table is deserialized

[wangyinsheng]

PTAL @nastra

[wangyinsheng]

PTAL @nastra

[wangyinsheng]

I did take another look and I don't think we need the changes in TestTables, because the tests will pass without those.

you're right, it not necessary. I used to compare the metadata between origin table and serializable table, then it will be needed. I have removed the changes, PTAL @nastra

[wangyinsheng]

PTAL @aokolnychyi

[wangyinsheng]

PTAL @aokolnychyi

[wangyinsheng]

could you please take a look this pr, it has last for weeks @aokolnychyi

[wangyinsheng]

@nastra Could you please approval the CI ? And is there someone else can review this pr?

[wangyinsheng]

@aokolnychyi @danielcweeks could you take a look please ? thanks

[nastra]

@wangyinsheng to fix CI failures I think you need to add the below code to TransactionTable:

    @Override
    public MetricsReporter metricsReporter() {
      return BaseTransaction.this.reporter;
    }

and then also this to BaseMetadataTable:

  @Override
  public MetricsReporter metricsReporter() {
    return table().metricsReporter();
  }

[nastra]

@wangyinsheng I talked to @rdblue yesterday and we concluded that we don't want to carry over the catalog properties just for the metrics reporter. The better approach would be to make MetricsReporter extend Serializable. I opened https://github.com/wangyinsheng/iceberg/pull/1 against your branch to do that. Feel free to squash it into your changes

[wangyinsheng]

@nastra I am confused why should't carry over the catalog properties for metrics reporter? We already support to config metrics report through catalog properites. And metrics reporter may contains non-serializable object, KafakProducer for example. We need properties to recreate non-serializable object after table been deserialized

[wangyinsheng]

please take a look @nastra @rdblue @aokolnychyi @danielcweeks

[wangyinsheng]

please take a look @nastra @rdblue @aokolnychyi @danielcweeks

[nastra]

@wangyinsheng as mentioned previously, we still would like to make the MetricsReporter Serializable. For things that aren't Serializable in a metrics reporter, we would make them transient and then lazily initialize them

[rdblue]

@nastra, where are we with the fix for serialization here?

[nastra]

@nastra, where are we with the fix for serialization here?

@rdblue I haven't heard back from @wangyinsheng after my comment above , so I've opened https://github.com/apache/iceberg/pull/7370 a while ago for initial experimentation and then updated it to fix the serialization issue.

[wangyinsheng]

@nastra, where are we with the fix for serialization here?

@rdblue I haven't heard back from @wangyinsheng after my comment above , so I've opened #7370 a while ago for initial experimentation and then updated it to fix the serialization issue.

@nastra @rdblue Sorry，I am working on other things for a while. But I am still keep my opinion, I think the SerializableTable is the best place to handle the lazily initialization stuff for MetricsReporter, we should not just make MetricsReporter serializable, which will throw the lazily initialization stuff to any implemention class of MetricsReporter

[github-actions[bot]]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

[github-actions[bot]]

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.