Skip to content

removed references of BEFORE/AFTER_COMMIT_VIEW #3554

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
snazy merged 2 commits into from
Jan 28, 2026
+3 −58
Merged

removed references of BEFORE/AFTER_COMMIT_VIEW #3554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c20e6a
removed references of BEFORE/AFTER_COMMIT_VIEW
evindj Jan 26, 2026
5879e3b
cleanup
evindj Jan 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti

### Breaking changes

- The (Before/After)CommitViewEvent has been removed.
- The (Before/After)CommitTableEvent has been removed.
- The `PolarisMetricsReporter.reportMetric()` method signature has been extended to include a `receivedTimestamp` parameter of type `java.time.Instant`.

Expand Down
20 changes: 0 additions & 20 deletions runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1852,16 +1852,6 @@ public void doRefresh() {
}

public void doCommit(ViewMetadata base, ViewMetadata metadata) {
polarisEventListener.onEvent(
new PolarisEvent(
PolarisEventType.BEFORE_COMMIT_VIEW,
eventMetadataFactory.create(),
new AttributeMap()
.put(EventAttributes.CATALOG_NAME, catalogName)
.put(EventAttributes.VIEW_IDENTIFIER, identifier)
.put(EventAttributes.VIEW_METADATA_BEFORE, base)
.put(EventAttributes.VIEW_METADATA_AFTER, metadata)));

// TODO: Maybe avoid writing metadata if there's definitely a transaction conflict
LOGGER.debug(
"doCommit for view {} with metadataBefore {}, metadataAfter {}",
Expand Down Expand Up @@ -1959,16 +1949,6 @@ public void doCommit(ViewMetadata base, ViewMetadata metadata) {
} else {
updateTableLike(identifier, entity);
}

polarisEventListener.onEvent(
new PolarisEvent(
PolarisEventType.AFTER_COMMIT_VIEW,
eventMetadataFactory.create(),
new AttributeMap()
.put(EventAttributes.CATALOG_NAME, catalogName)
.put(EventAttributes.VIEW_IDENTIFIER, identifier)
.put(EventAttributes.VIEW_METADATA_BEFORE, base)
.put(EventAttributes.VIEW_METADATA_AFTER, metadata)));
}

protected String writeNewMetadataIfRequired(ViewMetadata metadata) {
Expand Down
4 changes: 2 additions & 2 deletions runtime/service/src/main/java/org/apache/polaris/service/events/PolarisEventType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ public enum PolarisEventType {
AFTER_RENAME_VIEW,
BEFORE_REPLACE_VIEW,
AFTER_REPLACE_VIEW,
BEFORE_COMMIT_VIEW,
AFTER_COMMIT_VIEW,
BEFORE_COMMIT_VIEW, // REMOVED FROM SOURCE CODE
Copy link
Contributor

@adutra adutra Jan 27, 2026

Choose a reason for hiding this comment

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

I'm catching up here and saw this related conversation:

https://github.com/apache/polaris/pull/3195/changes#r2593440033

I'm not sure I agree with keeping these constants. Granted the ordinals will change if we remove them. But the feature is in beta status right now. It feels odd to see deprecated/unused constants in an API that is not even "official" yet.

@adnanhemani @dimas-b what do you think?

Copy link
Contributor

@dimas-b dimas-b Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I do not see any particular issue with changing ordinals. AFAIK, Polaris does not use java serialization for this enum and Jackson deals with names.

Copy link
Contributor Author

@evindj evindj Jan 27, 2026

Choose a reason for hiding this comment

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

If we are to remove the constants, will be ok doing in a follow up PR?cc @adutra I can take ownership of that issue as well.

Copy link
Member

@snazy snazy Jan 27, 2026

Choose a reason for hiding this comment

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

I think doing it in a separate PR makes sense, so the two other similar enum values can be removed as well.

Copy link
Member

@snazy snazy Jan 28, 2026

Choose a reason for hiding this comment

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

@evindj can you take care of the enum above in a separate PR?

Copy link
Contributor Author

@evindj evindj Jan 28, 2026

Choose a reason for hiding this comment

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

sure @snazy

Copy link
Contributor

@adutra adutra Jan 28, 2026

Choose a reason for hiding this comment

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

+1 for tackling this in a separate PR. Thanks @evindj !

AFTER_COMMIT_VIEW, // REMOVED FROM SOURCE CODE
BEFORE_REFRESH_VIEW,
AFTER_REFRESH_VIEW,

Expand Down
36 changes: 0 additions & 36 deletions .../test/java/org/apache/polaris/service/catalog/iceberg/AbstractIcebergCatalogViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,41 +267,5 @@ public void testEventsAreEmitted() {
Assertions.assertThat(
afterRefreshEvent.attributes().getRequired(EventAttributes.VIEW_IDENTIFIER))
.isEqualTo(TestData.TABLE);

PolarisEvent beforeCommitEvent =
testPolarisEventListener.getLatest(PolarisEventType.BEFORE_COMMIT_VIEW);
Assertions.assertThat(
beforeCommitEvent.attributes().getRequired(EventAttributes.VIEW_IDENTIFIER))
.isEqualTo(TestData.TABLE);
Assertions.assertThat(
beforeCommitEvent
.attributes()
.get(EventAttributes.VIEW_METADATA_BEFORE)
.map(m -> m.properties().get(key)))
.hasValue(valOld);
Assertions.assertThat(
beforeCommitEvent
.attributes()
.get(EventAttributes.VIEW_METADATA_AFTER)
.map(m -> m.properties().get(key)))
.hasValue(valNew);

PolarisEvent afterCommitEvent =
testPolarisEventListener.getLatest(PolarisEventType.AFTER_COMMIT_VIEW);
Assertions.assertThat(
afterCommitEvent.attributes().getRequired(EventAttributes.VIEW_IDENTIFIER))
.isEqualTo(TestData.TABLE);
Assertions.assertThat(
afterCommitEvent
.attributes()
.get(EventAttributes.VIEW_METADATA_BEFORE)
.map(m -> m.properties().get(key)))
.hasValue(valOld);
Assertions.assertThat(
afterCommitEvent
.attributes()
.get(EventAttributes.VIEW_METADATA_AFTER)
.map(m -> m.properties().get(key)))
.hasValue(valNew);
}
}