Uh oh!
There was an error while loading. Please reload this page.
TIdy cross DC code. The application and the solr module. - #4744
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
solr/cross-dc-manager/src/test/org/apache/solr/crossdc/manager/messageprocessor/TestMessageProcessor.java:73
openMocksreturns anAutoCloseableMockito session, but this return value is discarded, leaving one session open per test. The previousinitMockscall closed that session immediately; preserve that behavior here (or retain it in a field and close it from@After) to avoid leaking mock-maker state.
MockitoAnnotations.openMocks(this);
| * | ||
| * @param mirroredSolrRequest MirroredSolrRequest object that is being processed. | ||
| */ | ||
| void preventCircularMirroring(MirroredSolrRequest<?> mirroredSolrRequest) { |
There was a problem hiding this comment.
[Q] is this some deprecated function which earlier was being used?
its unused for now, that's why intelliJ flagged it but is it not useful anytime in future as well?
There was a problem hiding this comment.
This is currently handled individually in MirroringUpdateRequestProcessorFactory and in MirroringCollectionsHandler, but in both cases the code is much simpler than this method ... so I think we can drop it for now.
| private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); | ||
| static final String VERSION_FIELD = "_version_"; |
There was a problem hiding this comment.
these specific cleanups, i was also doing in this PR: #4610
There was a problem hiding this comment.
I just merged into this PR the changes from 4610! Thanks for calling it out.
| return required; | ||
| } | ||
| public String getDefaultValue() { |
There was a problem hiding this comment.
[Q] this getter may get used in future?
There was a problem hiding this comment.
My sense on that is YAGNI: You Ain't Going to Need It. If it was valuable, there would be tests around it...
| public void setSubmitTimeNanos(final long submitTimeNanos) { | ||
| this.submitTimeNanos = submitTimeNanos; | ||
| } | ||
There was a problem hiding this comment.
this is a setter for a private field. may be useful in future.
There was a problem hiding this comment.
Likewise the YAGNI argument...
There was a problem hiding this comment.
You can delete this, this should not be mutable anyway. BTW, the time tracking is broken anyway (nanos cannot be compared across wire) and needs to be fixed, I'll file a separate Jira.
| sb.append(", method=").append(solrRequest.getMethod()); | ||
| sb.append(", params=").append(solrRequest.getParams()); | ||
| if (solrRequest instanceof UpdateRequest req) { | ||
| sb.append(", add=" + (req.getDocuments() != null ? req.getDocuments().size() : "0")); | ||
| sb.append(", del=" + (req.getDeleteByIdMap() != null ? req.getDeleteByIdMap().size() : "0")); | ||
| sb.append(", dbq=" + (req.getDeleteQuery() != null ? req.getDeleteQuery().size() : "0")); | ||
| sb.append(", add=").append(req.getDocuments() != null ? req.getDocuments().size() : "0"); | ||
| sb.append(", del=") | ||
| .append(req.getDeleteByIdMap() != null ? req.getDeleteByIdMap().size() : "0"); | ||
| sb.append(", dbq=").append(req.getDeleteQuery() != null ? req.getDeleteQuery().size() : "0"); | ||
| } | ||
| sb.append(", attempt=" + attempt); | ||
| sb.append(", submitTimeNanos=" + submitTimeNanos); | ||
| sb.append(", attempt=").append(attempt); | ||
| sb.append(", submitTimeNanos=").append(submitTimeNanos); |
| package org.apache.solr.crossdc.update.processor; | ||
| /** Wrapper class for Mirroring exceptions. */ | ||
| public class MirroringException extends Exception { |
There was a problem hiding this comment.
[for-my-knowledge] why would this custom exception be created if this is not being used in the codebase?
any other usage outside this codebase possible for this, since it lies in cross-dc-manager module?
There was a problem hiding this comment.
I really don't know... Or it got created and never go used.. @sigram can you weigh in on this?
There was a problem hiding this comment.
Hmm, it is used in a few places... it's thrown by KafkaRequestMirroringHandler.mirror(request) and KafkaMirroringSink methods, although the callers catch it and convert either to SolrException or an error result.
There was a problem hiding this comment.
well thats odd... How the heck did I remove it as not used and/or have the tests pass??? Are KafkaRequestMirroringHandler and KafkaMirroringSink non Solr code?
There was a problem hiding this comment.
Regardless, happy to put it back... @sigram any other suggestions/preferences you have, please do speak up, you know this code much better than I!
There was a problem hiding this comment.
okay, there is a copy of MirroringException in solr/modules/cross-dc/src/java/org/apache/solr/crossdc/common/MirroringException.... Does that explain why this other one could be removed?
There was a problem hiding this comment.
Huh, yes .. The one in common should stay. This happened probably during the initial import when packages were restructured.
| mirroringHandler = new KafkaRequestMirroringHandler(sink); | ||
| } | ||
| private static Integer getIntegerPropValue(String name, Properties props) { |
There was a problem hiding this comment.
[Q] this may be useful in future right?
| verify(requestMirroringHandler, times(1)).mirror(updateRequest); | ||
| assertEquals("missing dbq", 1, updateRequest.getDeleteQuery().size()); | ||
| assertEquals("dbq value", "id:test*", updateRequest.getDeleteQuery().get(0)); | ||
| assertEquals("dbq value", "id:test*", updateRequest.getDeleteQuery().getFirst()); |
There was a problem hiding this comment.
there are other IntelliJ suggestions as well for this class, like this one?
There was a problem hiding this comment.
so I stayed away from that whole "can be final" and "can be local", mostly because I don't have the confidence of my convications that it IS the right change. It probably is, IntelliJ knows more than I do about Java syntax and keywords ;-). I was really focused on what I thought were the simplest cleanups cause I didn't want to create some weird lowlevel bug or race condition by changing attribute defintions.
Having said that, dealing with "field can be convert to a local variable" is prbably something that SHOULD be done in a seperate PR.
epugh
commented
Aug 28, 2026
I will leave this open for another day, and probably commit it on Monday Sept 1st. |
| * | ||
| * @param mirroredSolrRequest MirroredSolrRequest object that is being processed. | ||
| */ | ||
| void preventCircularMirroring(MirroredSolrRequest<?> mirroredSolrRequest) { |
There was a problem hiding this comment.
This is currently handled individually in MirroringUpdateRequestProcessorFactory and in MirroringCollectionsHandler, but in both cases the code is much simpler than this method ... so I think we can drop it for now.
| public void setSubmitTimeNanos(final long submitTimeNanos) { | ||
| this.submitTimeNanos = submitTimeNanos; | ||
| } | ||
There was a problem hiding this comment.
You can delete this, this should not be mutable anyway. BTW, the time tracking is broken anyway (nanos cannot be compared across wire) and needs to be fixed, I'll file a separate Jira.
epugh
commented
Aug 28, 2026
Uh oh!
There was an error while loading. Please reload this page.
(cherry picked from commit ae262ba)
Description
Go through code CrossDC code (both source and tests) and tidy up code.
Solution
Leverage Intellij warnings. Seperate from #4743 as I wanted to do the solr module and the application code together.
I may after this tackle the Kafka 4 upgrade for either solr 11 or solr 11 and 10 depending on feedback from folks who built this codebase.