Java update readme #636

acarbonetto · 2023-11-24T07:26:07Z

Issue #, if available: N/A

Description of changes:
Update the Java Client README.md to conform with other languages README templates.

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

Signed-off-by: Andrew Carbonetto <[email protected]>

Yury-Fridlyand · 2023-11-24T18:40:51Z

java/README.md

-1. A Java client (lib folder): wrapper to rust-client.
-2. An examples script: to sanity test javababushka and similar java-clients against a redis host.
-3. A benchmark app: to performance benchmark test javababushka and similar java-clients against a redis host.
+Tested on Amazon Corretto version 11.0.21, and 17.0.3


GHA uses Timurin SDK, you can referenct it too.
Mention which OS was tested on.

fixed in 2864a6e

Yury-Fridlyand · 2023-11-24T18:41:24Z

java/README.md

+- JDK 11+
+- git
+- protoc (protobuf compiler)
+- Gradle


gradle is not needed since we have gradlew

fixed in 2864a6e

Yury-Fridlyand · 2023-11-24T18:41:59Z

java/README.md

+- JDK 11+
+- git
+- protoc (protobuf compiler)
+- Gradle


But rust is needed

Good point... let me add it.

fixed in 2864a6e

Yury-Fridlyand · 2023-11-24T18:44:13Z

java/README.md

+sudo apt-get install openjdk-11-jdk
+VERSION=8.3
+wget https://services.gradle.org/distributions/gradle-${VERSION}-bin.zip -P /tmp
+sudo apt install -y protobuf-compiler


Suggested change

sudo apt-get install openjdk-11-jdk

VERSION=8.3

wget https://services.gradle.org/distributions/gradle-${VERSION}-bin.zip -P /tmp

sudo apt install -y protobuf-compiler

sudo apt-get install -y protobuf-compiler openjdk-11-jdk openssl gcc

last two are needed for rust and building rust dependencies

fixed in 2864a6e

java/README.md

Yury-Fridlyand · 2023-11-24T18:47:53Z

java/README.md

+* `./gradle :client:test` to run client unit tests
+* `./gradle :client:spotlessCheck` to check for codestyle issues
+* `./gradle :client:spotlessApply` to apply codestyle recommendations
+* `./gradle :benchmarks:run` to run performance benchmarks


Suggested change

* `./gradle :client:test` to run client unit tests

* `./gradle :client:spotlessCheck` to check for codestyle issues

* `./gradle :client:spotlessApply` to apply codestyle recommendations

* `./gradle :benchmarks:run` to run performance benchmarks

* `./gradlew :client:test` to run client unit tests

* `./gradlew :client:spotlessCheck` to check for codestyle issues

* `./gradlew :client:spotlessApply` to apply codestyle recommendations

* `./gradlew :benchmarks:run` to run performance benchmarks

Yury-Fridlyand · 2023-11-24T18:48:01Z

java/README.md

+   $ ./gradle :client:test
+   BUILD SUCCESSFUL
+    ```
+


integTest?

We can add it later

java/README.md

jonathanl-bq

I think it will be fine after Yury's comments have been addressed.

Signed-off-by: Andrew Carbonetto <[email protected]>

barshaul · 2023-11-28T07:51:37Z

java/README.md


-## Organization
+The beta release of Babushka was tested on Intel x86_64 using Ubuntu 22.04.1, and amd64 using macOS 14.1.


The Java wrapper won't be ready for the beta release, but we would have the java folder in the repository when we go public. So the README file should explain that the wrapper isn't ready yet and still a work in progress.
Lets remove the System Requirements, and avoid from saying that the java client was tested or is a part of the beta release - and instead lets add a Current Status section, explaining where we at.

Ok thanks. I've updated it, but please let me know what you think.

java/README.md

barshaul · 2023-11-28T08:07:44Z

java/README.md


-## Code style
+At the moment, the beta release of Babushka mush be build from source. 


it's not in beta yet, so it has to be built from source and not from package managers. lets change this sentence

Thanks. Please take a look at the updates if you can.

barshaul · 2023-11-28T08:11:06Z

java/README.md

+   Client testClient = new Client();
+
+   public RedisClient() {
+      testClient.asyncConnectToRedis("localhost", 6379, false, false).get(); // expect Ok


I would expect to have a constructor only with host and port, so the user won't have to pass 'false' flags

Right. We have to implement that, but I've updated the readme to reflect what we want.
updated in eb00378

barshaul · 2023-11-28T08:12:07Z

java/README.md

+
+   public RedisClient() {
+      testClient.asyncConnectToRedis("localhost", 6379, false, false).get(); // expect Ok
+      Response setResponse = testClient.asyncSet("name", "johnsmith").get(); // expect Ok


minor: lets stick to 'foo', 'bar', to match the other readmes

I used "key", "foobar".

barshaul · 2023-11-28T08:14:15Z

java/README.md

+
+   Client testClient = new Client();
+
+   public RedisClient() {


Can we remove the class and only include the client example, same as they do in Lettuce and Jedis?
https://github.com/lettuce-io/lettuce-core#basic-usage
https://github.com/redis/jedis#connecting-to-a-redis-cluster

removed in eb00378

barshaul · 2023-11-28T08:16:18Z

java/README.md

+   public RedisClient() {
+      testClient.asyncConnectToRedis("localhost", 6379, false, false).get(); // expect Ok
+      Response setResponse = testClient.asyncSet("name", "johnsmith").get(); // expect Ok
+      Response getResponse = testClient.asyncGet("name").get(); // expect "johnsmith"


maybe instead of adding as a comment, we can do same as lettuce did:

setResponse == "OK" getResponse == "bar"

updated in eb00378

barshaul · 2023-11-28T08:19:54Z

java/README.md

-* `host`: redis server host url
-* `port`: redis server port number
-* `tls`: redis TLS configured
-
 ### Troubleshooting


this troubleshooting is not specific for Java. We will add a general troubleshooting to the main wiki later on. Lets remove it from the Java README

removed in eb00378

barshaul · 2023-11-28T08:23:04Z

java/README.md

+    ```
+3. Generate protobuf files:
+    ```bash
+   $ cd java/


IMO removing the '$' and therefore also the 'BUILD SUCCESSFUL' will make it easier for users to copy the code to their terminals

updated in eb00378

Signed-off-by: Andrew Carbonetto <[email protected]>

barshaul

Overall looks good! Let's wait for our PM to approve the status message and then ship it

barshaul · 2023-11-29T08:04:33Z

java/README.md

-This module contains a Java-client wrapper that connects to the `Babushka`-rust-client. The rust client connects to
-redis, while this wrapper provides Java-language binding. The objective of this wrapper is to provide a thin-wrapper
-language api to enhance performance and limit cpu cycles at scale. 
+## Wrapper Status


Notice: Java Wrapper - Work in Progress

We're excited to share that the Java client is currently in development! However, it's important to note that this client is a work in progress and is not yet complete or fully tested. Your contributions and feedback are highly encouraged as we work towards refining and improving this implementation. Thank you for your interest and understanding as we continue to develop this Java wrapper.

Lets wait for Mickey, Asaf and Shachar to approve

Approved :)

Signed-off-by: Andrew Carbonetto <[email protected]>

acarbonetto added 2 commits November 23, 2023 23:19

Java: update README.md file

b497311

Signed-off-by: Andrew Carbonetto <[email protected]>

Java: update README.md file

8d40db6

Signed-off-by: Andrew Carbonetto <[email protected]>

acarbonetto requested a review from a team as a code owner November 24, 2023 07:26

acarbonetto requested review from jonathanl-bq, barshaul, Yury-Fridlyand and shachlanAmazon November 24, 2023 07:26

Yury-Fridlyand reviewed Nov 24, 2023

View reviewed changes

jonathanl-bq approved these changes Nov 24, 2023

View reviewed changes

acarbonetto added 2 commits November 27, 2023 13:11

Update java readme

2864a6e

Signed-off-by: Andrew Carbonetto <[email protected]>

Update java readme

d32d73c

Signed-off-by: Andrew Carbonetto <[email protected]>

barshaul requested changes Nov 28, 2023

View reviewed changes

Update java readme with disclaimer of WIP state

eb00378

Signed-off-by: Andrew Carbonetto <[email protected]>

barshaul approved these changes Nov 29, 2023

View reviewed changes

Update work in progress announcment

74cc451

Signed-off-by: Andrew Carbonetto <[email protected]>

barshaul merged commit b6f1d22 into valkey-io:main Dec 3, 2023

acarbonetto deleted the java_update_readme branch December 19, 2023 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java update readme #636

Java update readme #636

acarbonetto commented Nov 24, 2023

Yury-Fridlyand Nov 24, 2023

acarbonetto Nov 27, 2023

Yury-Fridlyand Nov 24, 2023

acarbonetto Nov 27, 2023

Yury-Fridlyand Nov 24, 2023

acarbonetto Nov 24, 2023

acarbonetto Nov 27, 2023

Yury-Fridlyand Nov 24, 2023

acarbonetto Nov 27, 2023

Yury-Fridlyand Nov 24, 2023

acarbonetto Nov 27, 2023

Yury-Fridlyand Nov 24, 2023

acarbonetto Nov 27, 2023

jonathanl-bq left a comment

barshaul Nov 28, 2023

acarbonetto Nov 28, 2023

barshaul Nov 28, 2023

acarbonetto Nov 28, 2023

barshaul Nov 28, 2023

acarbonetto Nov 28, 2023

barshaul Nov 28, 2023

acarbonetto Nov 28, 2023

barshaul Nov 28, 2023

acarbonetto Nov 28, 2023

barshaul Nov 28, 2023

acarbonetto Nov 28, 2023

barshaul Nov 28, 2023

acarbonetto Nov 28, 2023

barshaul Nov 28, 2023

acarbonetto Nov 28, 2023

barshaul left a comment

barshaul Nov 29, 2023

barshaul Nov 29, 2023

barshaul Nov 29, 2023


		## Organization
		The beta release of Babushka was tested on Intel x86_64 using Ubuntu 22.04.1, and amd64 using macOS 14.1.


		## Code style
		At the moment, the beta release of Babushka mush be build from source.

Java update readme #636

Java update readme #636

Conversation

acarbonetto commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanl-bq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Notice: Java Wrapper - Work in Progress

Choose a reason for hiding this comment

Choose a reason for hiding this comment