Skip to content

fix: Remove info log about deprecated internal method from PolarisConfiguration #1672

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
merged 4 commits into from
May 27, 2025
Merged

fix: Remove info log about deprecated internal method from PolarisConfiguration #1672

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c495080
Make the method supporting old config names "first class" code.
dimas-b May 23, 2025
cfa7a92
review: rename back to catalogConfigUnsafe
dimas-b May 26, 2025
499dde8
review: restore deprecation and javadoc
dimas-b May 27, 2025
52c68b4
add javadoc suggestion for alternatives to the deprecated method
dimas-b May 27, 2025
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
3 changes: 2 additions & 1 deletion polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,11 @@ public Builder<T> catalogConfig(String catalogConfig) {
/**
* Used to support backwards compatability before there were reserved properties. Usage of this
* method should be removed over time.
*
* @deprecated Use {@link #catalogConfig()} instead.
*/
@Deprecated
Copy link
Contributor

@eric-maynard eric-maynard May 26, 2025

Choose a reason for hiding this comment

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

I know the alerts are annoying, but I think we need to get in the habit of marking things we want to remove as Deprecated and would like to keep the annotation

Copy link
Contributor Author

@dimas-b dimas-b May 26, 2025

Choose a reason for hiding this comment

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

I'm not sure we want to remove this method. We may be able to remove it if we stop supporting the old config names at some point and never rename configs again 🤔

Copy link
Contributor

@eric-maynard eric-maynard May 26, 2025

Choose a reason for hiding this comment

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

That was definitely my intent when I made it -- to prohibit new usage and eventually phase out old usage. I think there's a comment to this effect, and the alerts are sort of "meant" to be annoying. We shouldn't have configs that are unsafe, I think.

Copy link
Contributor

@eric-maynard eric-maynard May 26, 2025

Choose a reason for hiding this comment

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

That's not to say we can't change our minds about this btw, just noting this is by design. Maybe bad design :)

Copy link
Contributor Author

@dimas-b dimas-b May 26, 2025

Choose a reason for hiding this comment

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

Annoying to whom? ;) Right now they seems to be very visible to use users, who cannot do anything to "fix" it 🤷

Copy link
Contributor

@eric-maynard eric-maynard May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Only to users building from source. Otherwise, it's meant to be visible to developers -- a reminder & incentive to deprecate the method, not one to just yank the @Deprecated annotation.

If we're concerned about the verbosity of ./gradlew build, these warnings are a very small fraction of the output I see when building. There is lots of other output which is much lower-signal to users, such as all the split package warnings or the Hibernate Validator output.

Copy link
Contributor Author

@dimas-b dimas-b May 27, 2025

Choose a reason for hiding this comment

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

As I noted in #1666 the concern is with log messages.

I'll restore the deprecation.

Copy link
Contributor Author

@dimas-b dimas-b May 27, 2025

Choose a reason for hiding this comment

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

This PR only removes the INFO log message now. Description updated.

Copy link
Contributor

@eric-maynard eric-maynard May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Just want to flag that there is a surviving log here, but IMO we should keep that one because it tells users to stop using the config, rather than developers to stop building Polaris with a method call in place they can't actually remove.

Copy link
Contributor Author

@dimas-b dimas-b May 27, 2025

Choose a reason for hiding this comment

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

The "surviving" log is quite appropriate IMHO. It informs the user that they ought to update their table's properties to use the new name.

public Builder<T> catalogConfigUnsafe(String catalogConfig) {
LOGGER.info("catalogConfigUnsafe is deprecated! Use catalogConfig() instead.");
if (catalogConfig.startsWith(SAFE_CATALOG_CONFIG_PREFIX)) {
throw new IllegalArgumentException(
"Unsafe catalog configs are not expected to start with " + SAFE_CATALOG_CONFIG_PREFIX);
Expand Down
Loading