Skip to content
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.

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

Nessie: Provide better commit message on table registation #8331 #8385

Merged
merged 2 commits into from Sep 11, 2023

Conversation

harshm-dev
Copy link
Contributor

@harshm-dev harshm-dev commented Aug 24, 2023

Small change covering the registration time commit message when the catalog is Nessie.

Fixes #8331

@@ -513,6 +513,8 @@ private String buildCommitMsg(TableMetadata base, TableMetadata metadata, String
"Iceberg %s against %s", metadata.currentSnapshot().operation(), tableName);
} else if (base != null && metadata.currentSchemaId() != base.currentSchemaId()) {
return String.format("Iceberg schema change against %s", tableName);
} else if (base == null) {
return String.format("Iceberg table registered with name %s", tableName);
Copy link
Member

Choose a reason for hiding this comment

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

Does create table also enter this flow? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does, sharing an example effect of this change:

Screenshot 2023-08-25 at 9 38 28 AM

Copy link
Member

Choose a reason for hiding this comment

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

So, we can't differentiate create table and register flow?

If there is no way, I am ok with having this commit message (which was better than previous)

Copy link
Member

Choose a reason for hiding this comment

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

IMO registered has a special meaning, I'd favor created/registered instead of just registered

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @snazy 's suggesion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, changed the message likewise created/registered instead of just registered.

So, we can't differentiate create table and register flow?
The nearest possibility is to consider the case as create if there are no snapshots in the new metadata. Since it may not always be true, not pursuing that path.

@harshm-dev
Copy link
Contributor Author

harshm-dev commented Aug 25, 2023

Screenshot 2023-08-25 at 9 38 28 AM

@harshm-dev
Copy link
Contributor Author

This PR has been in review for a while now. Is there anything I can do to make things move on this track?

@harshm-dev
Copy link
Contributor Author

Resolves #8331

@ajantha-bhat
Copy link
Member

cc: @nastra. @snazy, @dimas-b

@ajantha-bhat
Copy link
Member

PR title we don't mention issue number.
And PR description can have a last line as Fixes #8331

Just a style that Iceberg follows.

@harshm-dev
Copy link
Contributor Author

@nastra will need your help with the merge.

@nastra nastra merged commit f8093e0 into apache:master Sep 11, 2023
41 checks passed
@harshm-dev harshm-dev deleted the register_commit_msg branch September 11, 2023 11:21
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.

Improper commit message post table migration at the Nessie commit
5 participants