-
Notifications
You must be signed in to change notification settings - Fork 11
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
Added "mode" option for crud select operation #416
Conversation
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 for the PR. Great job, I left only two small comments
* @return this options instance. | ||
*/ | ||
default T withMode(String mode) { | ||
if (!mode.equals("read") && !mode.equals("write")) { |
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.
nit: You can use enum because modes are determined values. It's up to you
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 suggest using enums. This check will look better then. Also, the users will benefit from using the enum values that are better supported by autocompletion in editors.
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.
Also I do not like the initial naming, it's rather strange. We may think about using something more widely used in the industry, like leader
/replica
, otherwise the meaning is very obscure (my first thought was "is select
writing anything?!")
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.
Also I do not like the initial naming, it's rather strange. We may think about using something more widely used in the industry, like
leader
/replica
, otherwise the meaning is very obscure (my first thought was "isselect
writing anything?!")
It should have been renamed in crud/vshard. If we changed it in connector it would be confusuing thing for users who wants to understand how tarantool and ecosystem works. Let it be unified
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. Done.
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.
Looks good, I have two suggestions BTW
* @return this options instance. | ||
*/ | ||
default T withMode(String mode) { | ||
if (!mode.equals("read") && !mode.equals("write")) { |
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 suggest using enums. This check will look better then. Also, the users will benefit from using the enum values that are better supported by autocompletion in editors.
src/test/java/io/tarantool/driver/integration/proxy/options/ProxySpaceSelectOptionsIT.java
Outdated
Show resolved
Hide resolved
* @return this options instance. | ||
*/ | ||
default T withMode(String mode) { | ||
if (!mode.equals("read") && !mode.equals("write")) { |
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.
Also I do not like the initial naming, it's rather strange. We may think about using something more widely used in the industry, like leader
/replica
, otherwise the meaning is very obscure (my first thought was "is select
writing anything?!")
Needed for #107
I haven't forgotten about:
Related issues:
Closes #107