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-15111 Use JDK8 Base64 instead of own implementation #24

Merged
merged 2 commits into from
Aug 31, 2021

Conversation

asalamon74
Copy link
Contributor

Original lucene-solr pull request: apache/lucene-solr#2252 I removed the lucene part of it.

Description

JDK8 has a builtin Base64 encoder and decoder, there is no need to use own implementation for this.

Solution

Eliminate own implementation.

Tests

Unit tests

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

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

Looks mostly straight forward. But I think a few places can be simplified by calling encodeToString(<bytebuffer>.array()) rather than new String(encode(<bytebuffer>).array, <charset>), see comment.

@asalamon74
Copy link
Contributor Author

Rebased on top of the current main. @janhoy can you please take a look at it.

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

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

I have no further comments. If tests pass I’m happy :)

@janhoy
Copy link
Contributor

janhoy commented Aug 26, 2021

Please add a CHANGES.txt entry for version 8.10, and I’ll commit.

Have you run the entire test suite?

@asalamon74
Copy link
Contributor Author

@janhoy Added the CHANGES.txt entry. I executed ./gradlew check as far as I know it executes the tests.

@janhoy janhoy merged commit bd3c058 into apache:main Aug 31, 2021
epugh pushed a commit to epugh/solr that referenced this pull request Oct 22, 2021
Co-authored-by: Jan Høydahl <janhoy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants