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

Add Scala and Kotlin examples #1933

Merged

Conversation

jonathanl-bq
Copy link
Collaborator

Adds new examples for how to use Valkey GLIDE in both Scala and Kotlin projects. The Java README is also updated to show users how to use Valkey GLIDE in SBT projects.

@jonathanl-bq jonathanl-bq requested a review from a team as a code owner July 12, 2024 22:25
@jonathanl-bq jonathanl-bq added java issues and fixes related to the java client docs Documentation labels Jul 12, 2024
java/README.md Outdated Show resolved Hide resolved
@jonathanl-bq jonathanl-bq force-pushed the java/lotjonat_other_lang_examples branch from 1251b89 to 13ba768 Compare July 16, 2024 23:49
@@ -1 +0,0 @@
*.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is following Yury's suggestion to merge all .gitignores into a single .gitignore for all examples.

import kotlinx.coroutines.runBlocking
import java.util.concurrent.ExecutionException

suspend fun runApp() {
Copy link
Collaborator

@barshaul barshaul Jul 18, 2024

Choose a reason for hiding this comment

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

which app is it? it should be clear in the file/folder name or in the code itself that it tests a standalone client, and to provide cluster example as well. also i'm concern that users will do copy paste to their apps and it doesn't contain the error handling that we do in the main examples. Lets try to stick with one example format in #1925 and copy it exactly to the other java flavours (e.g. kotlin)

}
}

libraryDependencies += "io.valkey" % "valkey-glide" % "1.0.1" classifier platformClassifier
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't use hardcoded glide versions, keep it as an env variable for all examples and use it once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jonathanl-bq Try using version wildcard if possible, e.g. (`1+)


import java.util.concurrent.ExecutionException

object Main {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comments as I wrote in the kotlin example

java/README.md Outdated
- **IMPORTANT** must include a `classifier`. Please use this dependency block and add it to the build.sbt file.
```scala
// osx-aarch_64
libraryDependencies += "io.valkey" % "valkey-glide" % "1.0.1" classifier "osx-aarch_64"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same - don't use hardcoded glide version

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping

@@ -130,6 +130,22 @@ Maven:
</dependency>
```

SBT:
- **IMPORTANT** must include a `classifier`. Please use this dependency block and add it to the build.sbt file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the classifier is a must? shouldn't SBT decide based on the running platform?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the same as for Maven and Gradle, which both also have this note. There are plugins that can do this for each of the 3 build tools, but on its own, SBT cannot do this.

java/README.md Outdated
Comment on lines 267 to 277
#### Scala
To run the Scala example, from the `examples/scala/example` directory, execute:
```shell
sbt run
```

#### Kotlin
To run the Kotlin example, from the `examples/kotlin/example` directory, execute:
```shell
./gradlew run
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove it from here and add README file to each example folder that explains how to run the example


dependencies {
testImplementation(kotlin("test"))
implementation("io.valkey:valkey-glide:1.0.1:$classifier")
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't use glide hardcoded version

@@ -0,0 +1,234 @@
#!/bin/sh
Copy link
Collaborator

@barshaul barshaul Jul 18, 2024

Choose a reason for hiding this comment

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

Instead of storing all of the build files in the repo, is it possible to run in a build script "gradle init" or other relevant command to generate these files as a part of the example build process?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It requires gradle to be intalled on the computer. With gradlew you can bypass that and get any version of gradle at compile time.

Copy link
Collaborator

@barshaul barshaul left a comment

Choose a reason for hiding this comment

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

I think that it's great that you're adding also kotlin and scala examples!
We should make sure that the examples match the java example (see #1925 ), and lets try to make sure it doesn't have a lot of maintenance cost and keep it light as possible (can we get rid of all build files and generate them on user demand?), ), also see all comments inline.

@jonathanl-bq
Copy link
Collaborator Author

I think I've addressed all the comments now and aligned with Yi-Pin's merged Java examples.

@Yury-Fridlyand
Copy link
Collaborator

@barshaul round

java/README.md Outdated
libraryDependencies += "io.valkey" % "valkey-glide" % "1.0.0" classifier "osx-x86_64"

// linux-aarch_64
libraryDependencies += "io.valkey" % "valkey-glide" % "1.0.0" classifier "linux-aarch_64"
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's already an issue - above you wrote 1.0.1 and here 1.0.0. change it so we wont have hardcoded versions. and it's a must, use a variable once and set it only in one place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the documentation it does not make sense to use a variable. Every user will copy a different line depending on their platform and only one line. I will replace these all to use a wildcard.

// The client wasn't able to reestablish the connection within the given retries
Logger.log(Logger.Level.ERROR, "glide", s"Connection error encountered: ${e.getMessage}")
Future.failed(e)
case e: ExecAbortException =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

dont catch ExecAbortException. it should be catched as a part of the general Execution error.

* @param client An instance of <code>GlideClient</code>.
*/
private def appLogic(client: GlideClient): Future[Unit] = {
for {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is it inside a 'for'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Scala has for-comprehensions, which act as monad comprehensions (inspired by Haskell's do-notation) for writing imperative code with effects (Future is used to encode the async effect). It's idiomatic for Scala code to be written this way, even if the keyword "for" is a bit confusing, unfortunately.

```

To run the Cluster example:
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
```
```shell

Comment on lines 109 to 111
is ExecAbortException -> {
Logger.log(Logger.Level.ERROR, "glide", "ExecAbort error encountered: ${e.message}")
throw e
Copy link
Collaborator

Choose a reason for hiding this comment

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

same - ExecAbortException doesn't need specific error handling

Logger.log(Logger.Level.ERROR, "glide", "Connection error encountered: ${e.message}")
throw e
}
is ExecAbortException -> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@@ -0,0 +1,14 @@
# Run
Ensure that you have an instance of Valkey running on "localhost" on "6379". Otherwise, update StandaloneExample or ClusterExample with a configuration that matches your server settings.
Copy link
Collaborator

@barshaul barshaul Aug 1, 2024

Choose a reason for hiding this comment

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

Please fix it on all examples (including java's):

Suggested change
Ensure that you have an instance of Valkey running on "localhost" on "6379". Otherwise, update StandaloneExample or ClusterExample with a configuration that matches your server settings.
Ensure that you have a server running on "localhost" on port "6379". To run the ClusterExample, make sure that the server has cluster mode enabled. If the server is running on a different host and/or port, update the StandaloneExample or ClusterExample with a configuration that matches your server settings.

@@ -0,0 +1,14 @@
# Run
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a note to the readme files that the examples are running version X, and to change it they need to change Y in file Z?

// The client wasn't able to reestablish the connection within the given retries
Logger.log(Logger.Level.ERROR, "glide", s"Connection error encountered: ${e.getMessage}")
Future.failed(e)
case e: ExecAbortException =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

same

@jonathanl-bq jonathanl-bq force-pushed the java/lotjonat_other_lang_examples branch from a265f46 to a7533c7 Compare August 1, 2024 22:14
jonathanl-bq and others added 19 commits August 1, 2024 16:39
Signed-off-by: Jonathan Louie <[email protected]>
Signed-off-by: Jonathan Louie <[email protected]>
Signed-off-by: Jonathan Louie <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Jonathan Louie <[email protected]>
Signed-off-by: Jonathan Louie <[email protected]>
Signed-off-by: Jonathan Louie <[email protected]>
Signed-off-by: Jonathan Louie <[email protected]>
@jonathanl-bq jonathanl-bq force-pushed the java/lotjonat_other_lang_examples branch from a7533c7 to 5f6dc97 Compare August 1, 2024 23:40
@jonathanl-bq
Copy link
Collaborator Author

@barshaul round

@@ -0,0 +1,51 @@
### Kotlin ###
.gradle
build/
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: move out of ### Kotlin ### section, since this is not specific to Kotlin

val os = System.getProperty("os.name").toLowerCase
val platformClassifier = {
(os, System.getProperty("os.arch").toLowerCase) match {
case (mac, arm) if mac.contains("mac") && (arm.contains("aarch") || arm.contains("arm")) => "osx-aarch_64"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: rename each of these lines:

Suggested change
case (mac, arm) if mac.contains("mac") && (arm.contains("aarch") || arm.contains("arm")) => "osx-aarch_64"
case (osName, archName) if mac.contains("mac") && (arm.contains("aarch") || arm.contains("arm")) => "osx-aarch_64"

@acarbonetto acarbonetto merged commit 60f71af into valkey-io:main Aug 8, 2024
6 checks passed
@asafpamzn asafpamzn mentioned this pull request Aug 11, 2024
10 tasks
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Aug 12, 2024
* Add Scala and Kotlin examples

Signed-off-by: Guian Gumpac <[email protected]>

---------

Signed-off-by: Jonathan Louie <[email protected]>
Signed-off-by: Guian Gumpac <[email protected]>
Co-authored-by: Yury-Fridlyand <[email protected]>
Co-authored-by: Guian Gumpac <[email protected]>
Signed-off-by: Chloe Yip <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation java issues and fixes related to the java client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants