Skip to content

Core: Add update event for rewrite manifests#12627

Merged
nastra merged 1 commit into
apache:mainfrom
bryanck:rw-manifest-event
Mar 24, 2025
Merged

Core: Add update event for rewrite manifests#12627
nastra merged 1 commit into
apache:mainfrom
bryanck:rw-manifest-event

Conversation

@bryanck

@bryanck bryanck commented Mar 24, 2025

Copy link
Copy Markdown
Contributor

The rewrite manifests snapshot producer is the only one that does not generate an update event, thus listeners are not notified when a rewrite manifest occurs. This PR adds an update event for rewrite manifests.

@bryanck bryanck requested review from danielcweeks and nastra March 24, 2025 00:38
@github-actions github-actions Bot added the core label Mar 24, 2025
@bryanck bryanck force-pushed the rw-manifest-event branch from 1df4dbe to 1ec22e5 Compare March 24, 2025 01:24
@bryanck bryanck force-pushed the rw-manifest-event branch from 1ec22e5 to a9b17d5 Compare March 24, 2025 01:37
assertThat(report.sequenceNumber()).isEqualTo(3L);
assertThat(report.tableName()).isEqualTo(tableName);

CommitMetricsResult metrics = report.commitMetrics();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably change this to

CommitMetricsResult metrics = report.commitMetrics();
assertThat(metrics.totalDataFiles().value()).isEqualTo(2L);
assertThat(metrics.totalRecords().value()).isEqualTo(2L);
assertThat(metrics.totalFilesSizeInBytes().value()).isEqualTo(20L);

so that we at least check that some metrics were recorded

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the PR as-is. I'll follow up in a separate PR to add proper commit metrics for manifests kept/created/replaced

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryanck I've opened #12630 to address ^

@nastra nastra merged commit 6bd6887 into apache:main Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants