-
Notifications
You must be signed in to change notification settings - Fork 57
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
Conversation
There was a problem hiding this 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 + ":"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
...ava/dev/langchain4j/store/embedding/redis/spring/RedisEmbeddingStoreAutoConfigurationIT.java
Show resolved
Hide resolved
There was a problem hiding this 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!
Issue
Closes langchain4j#2336
Change
lombok
in Elasticsearch / Redis / Milvusmaven-source-plugin:test-jar-no-fork
forlangchain4j-spring-boot-tests
, in order to see test source in community repo.prefix
parameter introducing in this PRGeneral checklist