Configurable metrics reporter by catalog properties

[kmozaid]

No description provided.

[nastra]

@kmozaid now that #6404 is merged, can you please rebase this PR?

[kmozaid]

@kmozaid now that #6404 is merged, can you please rebase this PR?

Hi @nastra I have rebased and updated PR with feedbacks.

[nastra]

For testing the custom metrics reporter it would be good to add the following test to TestJdbcCatalog:

+ @Test
+ public void testCatalogWithCustomMetricsReporter() throws IOException {
+ catalog =
+ initCatalog(
+ "test_jdbc_catalog_with_custom_reporter",
+ ImmutableMap.of(
+ CatalogProperties.METRICS_REPORTER_IMPL, CustomMetricsReporter.class.getName()));
+
+ catalog.buildTable(TABLE, SCHEMA).create();
+ Table table = catalog.loadTable(TABLE);
+ table
+ .newFastAppend()
+ .appendFile(
+ DataFiles.builder(PartitionSpec.unpartitioned())
+ .withPath("/path/to/data-a.parquet")
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build())
+ .commit();
+
+ try (CloseableIterable<FileScanTask> tasks = table.newScan().planFiles()) {
+ assertThat(tasks.iterator()).hasNext();
+ }
+
+ // counter of custom metrics reporter should have been increased
+ // 1x for commit metrics / 1x for scan metrics
+ assertThat(CustomMetricsReporter.COUNTER.get()).isEqualTo(2);
+ }
+
+ public static class CustomMetricsReporter implements MetricsReporter {
+ static final AtomicInteger COUNTER = new AtomicInteger(0);
+
+ @Override
+ public void report(MetricsReport report) {
+ COUNTER.incrementAndGet();
+ }
+ }

[nastra]

@kmozaid while reviewing this PR I've noticed that not every catalog actually exposes its properties (including the JDBC catalog which I suggested above to use for testing). I've opened #6471 to address that

[nastra]

LGTM with some nits.
@rdblue could you review this one please?

[kmozaid]

Can I please get another review for merge to get enabled?

[PraveenNanda124]

Looks good

[kmozaid]

@gaborkaszab Can you please get this merged?

[gaborkaszab]

@gaborkaszab Can you please get this merged?

The patch LGTM, however, I won't be able to merge as I'm not a committer. cc @rdblue

[szehon-ho]

I think its fine with me if we can fix the failures

[kmozaid]

@gaborkaszab@szehon-ho Could you please trigger workflows?

[nastra]

@kmozaid could you rebase the PR please?
@danielcweeks could you review this please when you get a chance (and also trigger CI)?

[kmozaid]

@nastra I have rebased PR.

[nastra]

I'm confused, because the changes in this file should be gone after a rebase

[kmozaid]

sorry, I didn't build locally.
thanks, I agree it should be gone. Fixed it.

Merged, thanks @kmozaid , also @nastra , @gaborkaszab , @PraveenNanda124 for reviews