vyatkinv commented on PR #4320: URL: https://github.com/apache/solr/pull/4320#issuecomment-4366560558
> I suggest that you change StreamFactory to not mention "ZK" whatsoever, instead using the name we choose (e.g. getDefaultCloudString). For backwards-compatibility, however, the older method names should exist as deprecated. Okay, I implemented this. Regarding tests: I didn’t add new separate tests; instead, I created a method that randomly provides either an HTTP or Zookeeper record in different scenarios. Additionally, all zkHost parameters have been replaced with `connectionString`, using `getSolrConnection().toString()` for substitution. What do you think about this approach? While working on this, I ran into an issue with `StreamExpressionTest.testCloudSolrStreamWithZkHost`: `StreamExpressionParser.parse` splits parameters by commas, which breaks parsing for multi-host connection strings. ``` invalid expression search(collection1,solrConnection="http://127.0.0.1:34287/solr",http://127.0.0.1:40117/solr,http://127.0.0.1:46035/solr,http://127.0.0.1:43897/solr,q="*:*",fl="id,a_s,a_i,a_f",sort="a_f asc, a_i asc") - unknown operands found java.io.IOException: invalid expression search(collection1,solrConnection="http://127.0.0.1:34287/solr",http://127.0.0.1:40117/solr,http://127.0.0.1:46035/solr,http://127.0.0.1:43897/solr,q="*:*",fl="id,a_s,a_i,a_f",sort="a_f asc, a_i asc") - unknown operands found at __randomizedtesting.SeedInfo.seed([204B7454796EC30A:3B97CA6E50F5F12F]:0) at org.apache.solr.client.solrj.io.stream.CloudSolrStream.<init>(CloudSolrStream.java:117) at org.apache.solr.client.solrj.io.stream.StreamExpressionTest.testCloudSolrStreamWithZkHost(StreamExpressionTest.java:426) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1763) at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:946) at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:982) ``` I’d suggest discussing how to handle this: - keep the current behavior and accept the limitation that `solrConnection` in expressions can only contain a single host? - enhance the parser (e.g., support semicolon-separated values, special handling for `solrConnection`, or add support for quoting/escaping) ? For easier reproduction, I temporarily disabled the randomization and made it always use an HTTP quorum. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
