fix: Remove info log about deprecated internal method from PolarisConfiguration #1672
Conversation
dimas-b
requested review from
HonahX,
MonkeyCanCode,
RussellSpitzer,
adutra,
ajantha-bhat,
ashvina,
collado-mike,
dennishuo,
ebyhr,
eric-maynard,
flyrain,
jackye1995,
jbonofre,
pingtimeout,
singhpk234,
snazy,
takidau and
vvcephei
as code owners
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
There was a problem hiding this comment.
I still like unsafe because it suggests the unsafe nature of the method -- it's not clear to me what "legacy" means here. It's old, but, what's different about it?
There was a problem hiding this comment.
Legacy here means previously used config names.
I'm ok keeping the "unsafe" name. Will rename.
There was a problem hiding this comment.
renamed back to catalogConfigUnsafe (with javadoc update)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's not to say we can't change our minds about this btw, just noting this is by design. Maybe bad design :)
There was a problem hiding this comment.
Annoying to whom? ;) Right now they seems to be very visible to use users, who cannot do anything to "fix" it 🤷
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As I noted in #1666 the concern is with log messages.
I'll restore the deprecation.
There was a problem hiding this comment.
This PR only removes the INFO log message now. Description updated.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* Rename `catalogConfigUnsafe` to `legacyCatalogConfig` * Remove deprecation, because this method has a purpose in allowing backward compatibility when properties are renamed. * Remove the INFO log about calls to this method in Polaris, because users cannot do anything about these messages. Phasing out old property names required coordination with users (e.g. release notes), so it is not a matter of merely avoiding calls to that method in Polaris code. Fixes apache#1666
dimas-b
changed the title
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
* fix: Remove duplicated code in IcebergCatalog (apache#1681) * Fix SparkClient listGenericTable to use ListGenericTablesRESTResponse ListTableResponse is the class used by iceberg endpoint, which is the same as ListGenericTablesRESTResponse. However, we are suppose to use ListGenericTablesRESTResponse to be correct * fix: Remove info log about deprecated internal method from PolarisConfiguration (apache#1672) Remove the INFO log about calls to this method in Polaris, because users cannot do anything about these messages. Phasing out old property names requires coordination with users (e.g. release notes), so it is not a matter of merely avoiding calls to that method in Polaris code. Fixes apache#1666 * Create a single binary distribution bundle (apache#1589) * fix(quickstart): Correct Quickstart Instructions (apache#1673) * Remove Java URI validations for Blob Storage providers (apache#1604) This is a retry at apache#1586, which I'll close if this PR is on the right direction instead. Java URI does not actually apply any normalization to URIs if we do not call URI.normalize() (which we currently do not). Additionally, blob storage providers like S3 and GCS can provide ".." and "." as valid fragments in URLs - which Java URI would attempt to normalize incorrectly. As a result, attempting to validate and/or normalize URIs for blob storage providers using the Java URI class is the incorrect behavior. While we may want to add location validation via regex later, removing it first should at least unblock the bug we see in apache#1545. * Improve test coverage for invalid inputs in Policy APIs (apache#1665) * Fix getting-started docker start by PR apache#1532 (apache#1687) * Fix the manual test broken by PR apache#1532 (apache#1688) * Fix credentials printing twice (apache#1682) * main: Update dependency com.diffplug.spotless:spotless-plugin-gradle to v7.0.4 (apache#1690) * INFO: last merged commit 493de03 --------- Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: gh-yzou <[email protected]> Co-authored-by: Dmitri Bourlatchkov <[email protected]> Co-authored-by: Yufei Gu <[email protected]> Co-authored-by: Adnan Hemani <[email protected]> Co-authored-by: William Hyun <[email protected]> Co-authored-by: Christopher Lambert <[email protected]> Co-authored-by: Mend Renovate <[email protected]>
3 participants
Remove the INFO log about calls to this method in Polaris, because users cannot do anything about these messages.
Phasing out old property names requires coordination with users (e.g. release notes), so it is not a matter of merely avoiding calls to that method in Polaris code.
Fixes #1666