Skip to content
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

Java: Add GETBIT command #1400

Merged
merged 12 commits into from May 22, 2024
Merged

Conversation

GumpacG
Copy link
Contributor

@GumpacG GumpacG commented May 9, 2024

Issue #, if available:
N/A

Description of changes:
Implements GETBIT command in the java client.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@GumpacG GumpacG requested a review from a team as a code owner May 9, 2024 23:41
@Yury-Fridlyand Yury-Fridlyand added the java issues and fixes related to the java client label May 10, 2024
Copy link
Collaborator

@jonathanl-bq jonathanl-bq left a comment

Choose a reason for hiding this comment

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

Some minor doc comments, but otherwise LGTM.

Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

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

a couple of comments

* assert payload == 1L; // The second bit for string stored at "myKey1" is set to 1.
* }</pre>
*/
CompletableFuture<Long> getbit(String key, long offset);
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider something smaller than Long for the return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we only had Long and Double types for all number types. Would introducing Integer/Byte/Short cause any issues?

Copy link
Member

Choose a reason for hiding this comment

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

shouldnt it be a boolean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets stick with long until we have a consensus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per the discussion offline, we are going to use long. This can be switched in the future if a boolean or byte is preferred.

Copy link
Member

@shohamazon shohamazon left a comment

Choose a reason for hiding this comment

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

good job :)

* assert payload == 1L; // The second bit for string stored at "myKey1" is set to 1.
* }</pre>
*/
CompletableFuture<Long> getbit(String key, long offset);
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt it be a boolean?

java/integTest/src/test/java/glide/SharedCommandTests.java Outdated Show resolved Hide resolved
GumpacG and others added 12 commits May 21, 2024 11:55
* Added GETBIT command

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Applied spotless

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Removed duplicate test case

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

* Added comments to tests

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>

---------

Signed-off-by: Guian Gumpac <guian.gumpac@improving.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
…s.java

Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
…s.java

Co-authored-by: jonathanl-bq <72158117+jonathanl-bq@users.noreply.github.com>
@Yury-Fridlyand Yury-Fridlyand merged commit 4277b97 into aws:main May 22, 2024
47 of 48 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the java/integ_guiang_getbit branch May 22, 2024 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
java issues and fixes related to the java client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants