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

docs(memorystore): added valkey session snippets #10002

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dackers86
Copy link

Description

Fixes #

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

  • I have followed Sample Format Guide
  • pom.xml parent set to latest shared-configuration
  • Appropriate changes to README are included in PR
  • These samples need a new API enabled in testing projects to pass (let us know which ones)
  • These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
  • Tests pass: mvn clean verify required
  • Lint passes: mvn -P lint checkstyle:check required
  • Static Analysis: mvn -P lint clean compile pmd:cpd-check spotbugs:check advisory only
  • This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
  • This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
  • Please merge this PR for me once it is approved

@dackers86 dackers86 requested review from yoshi-approver and a team as code owners January 31, 2025 18:38
@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: memorystore labels Jan 31, 2025
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hi @dackers86 and reviewers,

I'm currently reviewing this pull request and will provide my feedback in a few minutes. In the meantime, I'm providing this summary of the pull request's content to help you and other reviewers quickly understand the changes and intent.

This pull request, "docs(memorystore): added valkey session snippets", adds several new code snippets demonstrating session management using Redis and Google Cloud Memorystore. The changes are focused on enhancing the documentation with practical examples.

Here's a breakdown of the changes:

  • New Files Added:

    • memorystore/valkey/session/snippets/pom.xml: A Maven POM file defining project dependencies, including Jedis (Redis client) and Google Cloud Core libraries. (Lines 1-117)
    • memorystore/valkey/session/snippets/src/main/java/samples/MemorystoreAddItemToBasket.java: A Java class demonstrating adding items to a user's basket in Memorystore. (Lines 1-68)
    • memorystore/valkey/session/snippets/src/main/java/samples/MemorystoreClearBasket.java: A Java class showing how to clear a user's basket. (Lines 1-60)
    • memorystore/valkey/session/snippets/src/main/java/samples/MemorystoreLoginUser.java: A Java class demonstrating user login and session creation. (Lines 1-61)
    • memorystore/valkey/session/snippets/src/main/java/samples/MemorystoreLogoutUser.java: A Java class for logging out a user by deleting their session. (Lines 1-58)
    • memorystore/valkey/session/snippets/src/main/java/samples/MemorystoreRemoveItemFromBasket.java: A Java class demonstrating removing items from a user's basket. (Lines 1-71)
  • Overall Intent: The goal is to provide clear, concise code examples illustrating common Memorystore use cases related to session management, such as login, logout, managing a shopping cart (basket), etc.

I'll be checking the code for correctness, style, and adherence to best practices. I'll also verify that the tests pass and that the documentation is up-to-date.


Here's a little poem to lighten the mood:

Code flows like a stream,
Tests run, a joyful green gleam,
Bugs flee in the night.

Looking forward to a smooth review!

Best,
Gemini

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This pull request adds new Java snippets for interacting with Memorystore for managing user sessions and shopping baskets. The code is generally well-structured and easy to follow. However, there are a few minor improvements that could enhance readability and maintainability. I've referenced the Google Java Style Guide in this review.

Key improvements recommended:

  • Using more descriptive variable names (e.g., sessionTimeoutSeconds instead of SESSION_TIMEOUT).
  • Adding more detailed Javadoc comments to explain the purpose and usage of each class and method.
  • Consistently applying try-with-resources for resource management.
  • Using parameterized logging to avoid unnecessary string concatenation.

Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

Your code samples do not follow the guidelines.
I am switching this PR to draft.
Please, adjust your code samples and resubmit the PR.

On the side note, what is a purpose of these code samples?
They show how to use the Redis client library to access Redis instance which is Memorystore under the hood. How are they different from code samples in the official Redis documentation?

Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

please add tests for each of your code samples.
review guidelines about testing framework and methods.
ensure that execution time for the tests is bound and short.
provision necessary objects and then clean up after the test is complete.
any sequential testing should be justified.
currently we implement two methods e2e tests that run vs testing environment and mocked tests. review other code samples for the reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: memorystore samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants