alessandrobenedetti commented on code in PR #3426:
URL: https://github.com/apache/solr/pull/3426#discussion_r2200099370
##########
solr/modules/llm/src/java/org/apache/solr/llm/textvectorisation/model/SolrTextToVectorModel.java:
##########
@@ -80,41 +84,40 @@ public static SolrTextToVectorModel getInstance(
* support, some of them may require to be handled in here as
separate switch cases
*/
switch (paramName) {
- case TIMEOUT_PARAM:
+ case TIMEOUT_PARAM -> {
Duration timeOut = Duration.ofSeconds((Long)
params.get(paramName));
builder.getClass().getMethod(paramName,
Duration.class).invoke(builder, timeOut);
- break;
- case MAX_SEGMENTS_PER_BATCH_PARAM:
- builder
- .getClass()
- .getMethod(paramName, Integer.class)
- .invoke(builder, ((Long) params.get(paramName)).intValue());
- break;
- case MAX_RETRIES_PARAM:
- builder
- .getClass()
- .getMethod(paramName, Integer.class)
- .invoke(builder, ((Long) params.get(paramName)).intValue());
- break;
+ }
+ case MAX_SEGMENTS_PER_BATCH_PARAM, MAX_RETRIES_PARAM -> builder
+ .getClass()
+ .getMethod(paramName, Integer.class)
+ .invoke(builder, ((Long) params.get(paramName)).intValue());
+
/*
* For primitive params if there's only one setter available, we
call it.
* If there's choice we default to the string one
*/
- default:
+ default -> {
ArrayList<Method> paramNameMatches = new ArrayList<>();
for (var method : builder.getClass().getMethods()) {
if (paramName.equals(method.getName()) &&
method.getParameterCount() == 1) {
paramNameMatches.add(method);
}
}
if (paramNameMatches.size() == 1) {
- paramNameMatches.get(0).invoke(builder, params.get(paramName));
+ paramNameMatches.getFirst().invoke(builder,
params.get(paramName));
} else {
- builder
- .getClass()
- .getMethod(paramName, String.class)
- .invoke(builder, params.get(paramName).toString());
+ try {
+ builder
+ .getClass()
+ .getMethod(paramName, String.class)
+ .invoke(builder, params.get(paramName).toString());
+ } catch (NoSuchMethodException e) {
+ log.error("Parameter {} not supported by model {}",
paramName, className);
+ throw e;
Review Comment:
Maybe better here to throw a SolrException wrapping it?
##########
solr/modules/llm/src/test/org/apache/solr/llm/textvectorisation/update/processor/TextToVectorUpdateProcessorFactoryTest.java:
##########
@@ -132,7 +132,7 @@ public void
init_notDenseVectorOutputField_shouldThrowExceptionWithDetailedMessa
/* Following test depends on a real solr schema and depends on
BeforeClass-AfterClass methods */
@Test
- public void
init_notExistentInputField_shouldThrowExceptionWithDetailedMessage() {
+ public void init_notExistentInputField_shouldNotThrowException() {
Review Comment:
mmmm I think this was ok, if the field is undefined I expect the exception.
You should keep this test and add a new one specific for a dynamic field
(the scope of this issue).
Did I misunderstand anything?
##########
solr/modules/llm/src/test/org/apache/solr/llm/textvectorisation/update/processor/TextToVectorUpdateProcessorTest.java:
##########
@@ -40,6 +41,13 @@ public static void cleanup() throws Exception {
afterTest();
}
+ @After
Review Comment:
It makes sense, have you checked that all is fine? I remember I struggled a
bit with the after/afterClass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
