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

Elasticsearch / Redis / Milvus: Remove lombok and small update for IT #101

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

Martin7-1
Copy link
Contributor

@Martin7-1 Martin7-1 commented Dec 26, 2024

Issue

Closes langchain4j#2336

Change

  1. Remove lombok in Elasticsearch / Redis / Milvus
  2. Configure maven-source-plugin:test-jar-no-fork for langchain4j-spring-boot-tests, in order to see test source in community repo.
  3. Redis: update prefix parameter introducing in this PR

General checklist

  • There are no breaking changes
  • I have added unit and/or integration tests for my change
  • The tests cover both positive and negative cases
  • I have manually run all the unit and integration tests in the module I have added/changed, and they are all green

Copy link
Owner

@langchain4j langchain4j left a comment

Choose a reason for hiding this comment

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

@Martin7-1 thank you!

@@ -30,6 +30,7 @@ public RedisEmbeddingStore redisEmbeddingStore(RedisEmbeddingStoreProperties pro
@Nullable EmbeddingModel embeddingModel) {
String host = Optional.ofNullable(properties.getHost()).orElse(DEFAULT_HOST);
int port = Optional.ofNullable(properties.getPort()).orElse(DEFAULT_PORT);
String prefix = Optional.ofNullable(properties.getPrefix()).orElse(DEFAULT_INDEX_NAME + ":");
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't default value for prefix embedding:? Using DEFAULT_INDEX_NAME (langchain4j-index) is a breaking change, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah...You are right. Maybe we should also remove the default value of indexName in spring boot starter, WDYT?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I guess defaults should be specified in the "core" lc4j modules, not in the spring boot starter

Copy link
Owner

Choose a reason for hiding this comment

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

But if you are going to remove anything, please make sure those are not breaking changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I only remove prefix for now.

Copy link
Owner

@langchain4j langchain4j left a comment

Choose a reason for hiding this comment

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

@Martin7-1 thank you!

@langchain4j langchain4j merged commit c4fccf5 into langchain4j:main Jan 3, 2025
1 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Make generic test source available
2 participants