-
Notifications
You must be signed in to change notification settings - Fork 67
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
Rename QueryIdCachingProxyHandler to Constants.java #504
base: main
Are you sure you want to change the base?
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Riya John.
|
Thanks for the submission @RiyaJohn, this looks like it can be merged once cla-bot is happy. My only question is if we want to add a |
gateway-ha/src/main/java/io/trino/gateway/ha/util/Constants.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/util/Constants.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/util/Constants.java
Outdated
Show resolved
Hide resolved
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.
LGTM if we rename the class name.
I think we should have it as a separate class because this is Trino http spec, not gateway.
gateway-ha/src/main/java/io/trino/gateway/ha/util/Constants.java
Outdated
Show resolved
Hide resolved
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.
Having a generic util package and a generic Constants class is an antipattern .. it will just end up as a big undefined class with no focus and spread into the codebase all over as a dependency. Please keep it focussed like in the original issues from @ebyhr
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Riya John.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Riya John.
|
@ebyhr I think I have addressed the review comments, please take a look at it. And I need help with this cla-signing. I have an email configured and I can see it in the git config, so not sure what is missing |
@willmostly @Chaho12 @vishalya @ebyhr @mosabua Can someone help me here ^^^ |
@RiyaJohn .. you need to rebase your branch and squash the commits into one commit. In addition you need to submit a signed CLA document and then wait for it to be verified and processed. Once that is done your github user name will show up in https://github.com/trinodb/cla/blob/master/contributors |
Description
Fixes #480
Release notes