-
Notifications
You must be signed in to change notification settings - Fork 270
Make PolarisConfiguration member variables private #2007
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
Conversation
CC @dimas-b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @poojanilangekar ! LGTM 👍
public String key() { | ||
return key; | ||
} | ||
|
||
public String description() { | ||
return description; | ||
} | ||
|
||
public T defaultValue() { | ||
return defaultValue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there actually a use case for method/nonvalue implementations of these? I notice the methods are not final
.
If not, it might not be worth breaking the interface and adding the extra overhead for callers. The values were already final
so there's no risk of someone changing a config in an unexpected way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding @dimas-b's comment here, (and let him address your question):
Sorry, I did not notice this before, but I think having public fiends in general is not a good idea. Since you're toughing this code in current PR, would you mind following up with another PR to convert them to private fields?
I agree that it's probably a bad idea to have public member variables, but don't feel strongly either way.
The other part that I'd be concerned about is the inconsistent public/private handling of fields. Since the fields are all final
, why shouldn't catalogConfigImpl
, catalogConfigUnsafeImpl
, and typ
be public
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having public fiends in general is not a good idea.
I'm not sure if I find that a compelling enough reason to make this change. It's generally considered a good idea because it affords you more flexibility to change the behavior of e.g. key()
without breaking an API, but
- In this case, we're proposing to actually break the API by removing public field
key
- Do we actually want to support that kind of flexibility? If so let's make this change, but originally my intent was to enforce that the child types have only a "plain" implementation of e.g.
key
.
Since the fields are all final, why shouldn't catalogConfigImpl, catalogConfigUnsafeImpl, and typ be public?
Those fields don't have a use outside this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those fields don't have a use outside this class.
That's inaccurate, there are several places in the code where catalogConfig()
is invoked. Which is simply trying to access the catalogConfigImpl
if present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any java symbols inside polaris-core
do not have backward-compatibility guaranteed per https://polaris.apache.org/in-dev/unreleased/evolution/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking the same in my original comment, if the accessors are public final
then we at least do not have to worry about divergence in the immediate concern and then the only real issue is that we're changing the API for seemingly no value.
Break points on a field do not work well in my experience (e.g. they do not catch reflected access, IIRC).
Watches are generally used for fields, not breakpoints.
Reflection and proxying can cause issues for breakpoints both fields and methods, which is one reason to avoid them. Unfortunately, you can observe this when working with method breakpoints in many proxied objects in Polaris today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eric-maynard : would you be ok with making these fields private
and their accessor methods public final
? Just to confirm :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@poojanilangekar : would you mind applying this suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
public String key() { | ||
return key; | ||
} | ||
|
||
public String description() { | ||
return description; | ||
} | ||
|
||
public T defaultValue() { | ||
return defaultValue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public
fields are generally considered bad practice, carefully designed interfaces are generally preferred. This is how things are composable and provide much better backwards compatibility. There are a lot of prominent examples in Java's core libraries. (Spoiler: Would anyone expect java.util.Map.Entry.key
being a field?)
In the past, a lot of changes went into Polaris that count as "breaking changes" according to the above arguments. Some received strong objections while others were just approved.
@poojanilangekar : it looks like some direct field access still exists (compilation errors) 🤔 some parallel code changes perhaps... |
This is strange, the build passed on my machine. I will try and fix it. |
ca47130
to
889b158
Compare
Turns out I just needed to rebase. |
If no other feedback, I'll merge tomorrow. |
As pointed out in #1959, this change converts all public member variable to private variables.