Skip to content
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

SOLR-16803: Remove dependencies on SimplePostTool from Solr Core classes.. #1805

Merged
merged 13 commits into from
Jul 31, 2023

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Jul 22, 2023

https://issues.apache.org/jira/browse/SOLR-16803

Description

Over time various classes through out Solr have dependencies on methods in the SimplePostTool class. However, this is sort of an ancillary class, and it shouldn't be relied upon.

Solution

Extract methods that are used into a new BinaryUtils.java file. I could also see the three methods being added to the existing "Utils.java" too....

Tests

Rerunning tests and bats.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@epugh epugh marked this pull request as draft July 22, 2023 21:42
@epugh epugh marked this pull request as ready for review July 22, 2023 22:41
@epugh epugh requested a review from risdenk July 24, 2023 11:02
import java.nio.BufferOverflowException;
import java.nio.ByteBuffer;

public class BinaryUtils {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is similar code in

solr/solrj/src/java/org/apache/solr/client/solrj/impl/BinaryRequestWriter.java:87:  public static class BAOS extends ByteArrayOutputStream {

Might make sense to use this class there as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the change to combine them in c486782, and it ended up being a bit more intrusive then I expected. All tests pass. Can you give it a quick look?

solr/core/src/java/org/apache/solr/util/BinaryUtils.java Outdated Show resolved Hide resolved
solr/core/src/java/org/apache/solr/util/BinaryUtils.java Outdated Show resolved Hide resolved
@epugh
Copy link
Contributor Author

epugh commented Jul 24, 2023

Great feedback @risdenk, especially on coming up with a better name ;-)

…tils

Required a move to the solrj/../common package in order to be used by solrj and core modules.
@epugh epugh requested a review from risdenk July 25, 2023 17:14
@epugh epugh merged commit ecf2561 into apache:main Jul 31, 2023
3 checks passed
epugh added a commit that referenced this pull request Jul 31, 2023
…ses.. (#1805)

Classes through solr/core and solr/solrj depended on methods defined in the SimplePostTool, a auxillary class that isn't really "part" of the core Solr offering.  Relocate those methods to the common Utils.java class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants