You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "markrmiller (via GitHub)" <gi...@apache.org> on 2023/04/03 16:37:33 UTC
[GitHub] [solr-sandbox] markrmiller opened a new pull request, #53: Move batching from solr+kafka to just kafka.
markrmiller opened a new pull request, #53:
URL: https://github.com/apache/solr-sandbox/pull/53
Make consumer multi-threaded.
Fix flakey tests.
Doc rewrite.
Additional mock unit tests.
Multiple topics per consumer support.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] gerlowskija commented on a diff in pull request #53: Move batching from solr+kafka to just kafka.
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #53:
URL: https://github.com/apache/solr-sandbox/pull/53#discussion_r1161731667
##########
SolrAndKafkaIntegrationTest.java:
##########
@@ -0,0 +1,335 @@
+package org.apache.solr.crossdc;
Review Comment:
[Q] I'm assuming this is just a temp file you copied to the root of the repo while debugging?
I spent a good 10 minutes trying to figure out why my copy of SolrAndKafkaIntegrationTest didn't have all the commented out methods in this file, and then I noticed that this copy is at the repo root and not the file that my IDE was pulling up 😬
##########
crossdc-producer/src/test/java/org/apache/solr/crossdc/SolrAndKafkaIntegrationTest.java:
##########
@@ -210,19 +211,25 @@ public void testProducerToCloud() throws Exception {
@Test
public void testMirroringUpdateProcessor() throws Exception {
final SolrInputDocument tooLargeDoc = new SolrInputDocument();
- tooLargeDoc.addField("id", "tooLarge-" + String.valueOf(System.currentTimeMillis()));
+ tooLargeDoc.addField("id", System.nanoTime());
tooLargeDoc.addField("text", new String(new byte[2 * MAX_DOC_SIZE_BYTES]));
final SolrInputDocument normalDoc = new SolrInputDocument();
- normalDoc.addField("id", "normalDoc-" + String.valueOf(System.currentTimeMillis()));
+ normalDoc.addField("id", System.nanoTime());
normalDoc.addField("text", "Hello world");
- final List<SolrInputDocument> docsToIndex = new ArrayList<>();
- docsToIndex.add(tooLargeDoc);
+ List<SolrInputDocument> docsToIndex = new ArrayList<>();
docsToIndex.add(normalDoc);
+ docsToIndex.add(tooLargeDoc);
final CloudSolrClient cluster1Client = solrCluster1.getSolrClient();
- cluster1Client.add(docsToIndex);
+ try {
+ cluster1Client.add(docsToIndex);
+ } catch (BaseCloudSolrClient.RouteException e) {
+ // expected
Review Comment:
[Q] Why is a RouteException expected here? If it's actually expected/desired shouldn't we assert to make sure it looks the way we expect?
##########
crossdc-producer/src/test/java/org/apache/solr/crossdc/SolrAndKafkaIntegrationTest.java:
##########
@@ -64,19 +62,24 @@
protected static volatile MiniSolrCloudCluster solrCluster1;
protected static volatile MiniSolrCloudCluster solrCluster2;
- protected static volatile Consumer consumer = new Consumer();
+ protected static volatile Consumer consumer;
private static String TOPIC = "topic1";
private static String COLLECTION = "collection1";
private static String ALT_COLLECTION = "collection2";
+ private static Thread.UncaughtExceptionHandler uceh;
@BeforeClass
Review Comment:
[-1/Q] Each test case in this class relies pretty heavily on the assumption that when the test-method starts, COLLECTION will have 0 docs on both primary and secondary.
Won't moving the Solr cluster creation/deletion logic from Before/After to BeforeClass/Afterclass break that assumption everywhere? Or am I overlooking somewhere that deletes-all-docs or deletes/recreates collections in between test methods?
##########
crossdc-producer/src/test/java/org/apache/solr/crossdc/SolrAndKafkaIntegrationTest.java:
##########
@@ -210,19 +211,25 @@ public void testProducerToCloud() throws Exception {
@Test
public void testMirroringUpdateProcessor() throws Exception {
final SolrInputDocument tooLargeDoc = new SolrInputDocument();
- tooLargeDoc.addField("id", "tooLarge-" + String.valueOf(System.currentTimeMillis()));
+ tooLargeDoc.addField("id", System.nanoTime());
Review Comment:
[Q] What's the purpose of the doc-id change here - were you running into conflicts? If you don't find it useful, fair enough, but I found the "tooLarge-" and "normalDoc-" prefixes here useful for debugging when I initially wrote this test.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] markrmiller commented on a diff in pull request #53: Move batching from solr+kafka to just kafka.
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #53:
URL: https://github.com/apache/solr-sandbox/pull/53#discussion_r1160804805
##########
crossdc-producer/src/test/resources/log4j2.xml:
##########
Review Comment:
Yeah, that was already there in the wip branch, when running tests log4j will pick that up instead of the production configuration.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] markrmiller commented on pull request #53: Move batching from solr+kafka to just kafka.
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on PR #53:
URL: https://github.com/apache/solr-sandbox/pull/53#issuecomment-1502064759
There is still one test issue that seems to perhaps be environment issue related, but I'm going to put this in for now and will continue to look into that.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] markrmiller commented on a diff in pull request #53: Move batching from solr+kafka to just kafka.
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #53:
URL: https://github.com/apache/solr-sandbox/pull/53#discussion_r1161995956
##########
crossdc-producer/src/test/java/org/apache/solr/crossdc/SolrAndKafkaIntegrationTest.java:
##########
@@ -64,19 +62,24 @@
protected static volatile MiniSolrCloudCluster solrCluster1;
protected static volatile MiniSolrCloudCluster solrCluster2;
- protected static volatile Consumer consumer = new Consumer();
+ protected static volatile Consumer consumer;
private static String TOPIC = "topic1";
private static String COLLECTION = "collection1";
private static String ALT_COLLECTION = "collection2";
+ private static Thread.UncaughtExceptionHandler uceh;
@BeforeClass
Review Comment:
I've switched back and forth a couple times trying to figure a fail I get with testMirroringUpdateProcessor, so it may have been off in the iteration that was up, I'm not sure, that latest is back to beforeclass, afterclass because I moved the test method that prompted that change initially into its own test class anyway.
The issue I've been seeing is unrelated though - the doc that goes missing in the secondary dc is dealing with collection2 and does get cleaned up in the test method itself, and it also is an issue even when just run on it's own for me.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] HoustonPutman commented on a diff in pull request #53: Move batching from solr+kafka to just kafka.
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #53:
URL: https://github.com/apache/solr-sandbox/pull/53#discussion_r1156248878
##########
crossdc-producer/src/test/resources/log4j2.xml:
##########
Review Comment:
Did you mean to commit this?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] markrmiller merged pull request #53: Move batching from solr+kafka to just kafka.
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller merged PR #53:
URL: https://github.com/apache/solr-sandbox/pull/53
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] HoustonPutman commented on a diff in pull request #53: Move batching from solr+kafka to just kafka.
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #53:
URL: https://github.com/apache/solr-sandbox/pull/53#discussion_r1156250608
##########
SolrAndKafkaIntegrationTest.java:
##########
Review Comment:
I think this is a mistake as well, since its also in the test dir...
##########
crossdc-consumer/src/main/java/org/apache/solr/crossdc/consumer/KafkaCrossDcConsumer.java:
##########
@@ -220,14 +285,97 @@ boolean pollAndProcessRequests() {
return true;
}
+ public void sendBatch(UpdateRequest solrReqBatch, ConsumerRecord<String,MirroredSolrRequest> lastRecord, WorkUnit workUnit) {
+ UpdateRequest finalSolrReqBatch = solrReqBatch;
+ Future<?> future = executor.submit(() -> {
+ IQueueHandler.Result<MirroredSolrRequest> result = messageProcessor.handleItem(new MirroredSolrRequest(finalSolrReqBatch));
+ try {
+ processResult(lastRecord, result);
+ } catch (MirroringException e) {
+ // We don't really know what to do here
+ log.error("Mirroring exception occurred while resubmitting to Kafka. We are going to stop the consumer thread now.", e);
Review Comment:
So this is a real error because kafka thinks the updates are processed, but there were Solr errors and we can't resubmit?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr-sandbox] markrmiller commented on a diff in pull request #53: Move batching from solr+kafka to just kafka.
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #53:
URL: https://github.com/apache/solr-sandbox/pull/53#discussion_r1160803458
##########
crossdc-consumer/src/main/java/org/apache/solr/crossdc/consumer/KafkaCrossDcConsumer.java:
##########
@@ -220,14 +285,97 @@ boolean pollAndProcessRequests() {
return true;
}
+ public void sendBatch(UpdateRequest solrReqBatch, ConsumerRecord<String,MirroredSolrRequest> lastRecord, WorkUnit workUnit) {
+ UpdateRequest finalSolrReqBatch = solrReqBatch;
+ Future<?> future = executor.submit(() -> {
+ IQueueHandler.Result<MirroredSolrRequest> result = messageProcessor.handleItem(new MirroredSolrRequest(finalSolrReqBatch));
+ try {
+ processResult(lastRecord, result);
+ } catch (MirroringException e) {
+ // We don't really know what to do here
+ log.error("Mirroring exception occurred while resubmitting to Kafka. We are going to stop the consumer thread now.", e);
Review Comment:
This is from the original code we contributed, but it should be that we failed sending to solr and we failed re queing to Kafka, so it seems everything is down.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org