[FLINK-10166] [table] Reduce dependencies by removing org.apache.commons #6966
Conversation
igalshilman
left a comment
igalshilman
left a comment
There was a problem hiding this comment.
This looks good to me, makes total senes to "inline" few methods and drop the dependencies 👍
Apart from few inline comments, LGTM.
|
|
||
| private static final Base64.Decoder BASE64_DECODER = java.util.Base64.getUrlDecoder(); | ||
|
|
||
| private static final MessageDigest MD5_MESSAGE_DIGEST = getMd5MessageDigest(); |
There was a problem hiding this comment.
Would MD5_MESSAGE_DIGEST be accessed concurrently? afaik this is not thread safe.
There was a problem hiding this comment.
Usually this doesn't matter for translation (which is single threaded) but we are on the safer side if this method is thread-safe in the future.
There was a problem hiding this comment.
Thanks for addressing this.
| */ | ||
| public static String repeat(final char ch, final int repeat) { | ||
| final char[] buf = new char[repeat]; | ||
| for (int i = repeat - 1; i >= 0; i--) { |
There was a problem hiding this comment.
I wonder why the original author has decided to loop backwards :-)
| } | ||
| return new String(output2); | ||
| default: | ||
| final StringBuilder buf = new StringBuilder(outputLength); |
There was a problem hiding this comment.
I know that you've just copied it, but honestly, I think that the default case is the only one that makes senes and all the complication in that method is a bit wired (as the comment above states - it is optimized for jdk1.4)
Feel free to ignore my comment, if you feel that fixing this, is out side of the scope of this PR :-)
There was a problem hiding this comment.
If I change this method, I would also need to introduce more extensive tests in order to verify that I did not introduce new bugs. This method can be removed soon anyway as Java 11 has a String.repeat method.
|
LGTM 👍 |
|
Thank you @igalshilman. I will merge this... |
This commit removes all dependencies to org.apache.commons libraries in flink-table. In the past we only used a couple of methods that were partially pulled in from Hadoop causing the issues mentioned in the JIRA ticket. This closes apache#6966.
flinkbot
added
component=TableSQL/API
component=TableSQL/Client
and removed
component=API/TableSQL
component=SQL/Client
labels
4 participants
What is the purpose of the change
This PR removes all dependencies to
org.apache.commonslibraries. In the past we only used a couple of methods that were partially pulled in from Hadoop causing the issues mentioned in the JIRA ticket.Brief change log
Verifying this change
This change is already covered by existing tests.
I added additional test such as
EncodingUtilsTestandTypeCheckUtilsTest.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation