replace guava functionality with equivalent JDK or custom features #319
Conversation
…ich allows a large (2.7MB) dependency to be dropped from size-sensitive deployments
There was a problem hiding this comment.
LGTM since the CI passes, but haven't read the code changes in details. Looking at the project history the guava dep was added very early to take advantage of a few specific types (6c89538), should be no harm replacing them with custom implementations.
Would definitely like @truthbk's review on this though, so leaving approval up to him.
|
The failing test ( |
|
There are some follow up PRs which make sense if they're all merged in the order of creation. If any of them are problematic the commits can be dropped and the PRs can be merged in isolation. |
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.*; |
There was a problem hiding this comment.
minor: afaik the styleguide says don't use wildcard imports?
There was a problem hiding this comment.
doesn't apply to test
There was a problem hiding this comment.
Actually this is up to @truthbk - wildcard imports (and some unused imports) are common in the test folder.
There was a problem hiding this comment.
Feel free to ignore but @mcculls is right, I know we already have a bunch of wildcard imports in the test package, but let's try to avoid adding more, if possible.
There was a problem hiding this comment.
ok will address this
| @@ -0,0 +1,47 @@ | |||
| package org.datadog.jmxfetch.util; | |||
|
|
|||
| public final class ByteArraySearcher { | |||
There was a problem hiding this comment.
interesting matching technique - I think it would be worth adding a small description of the algorithm used here
There was a problem hiding this comment.
This is the literal version of the bitap algorithm which is the only one I know which takes less space to write than the naive quadratic algorithm used in Guava.
| } | ||
| } | ||
| if (count.get() <= max) { | ||
| // may log a little too often if there are races |
There was a problem hiding this comment.
I think this is fine, not a huge concern.
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.*; |
There was a problem hiding this comment.
Feel free to ignore but @mcculls is right, I know we already have a bunch of wildcard imports in the test package, but let's try to avoid adding more, if possible.
The motivation here is to reduce the deployment size by shedding a 2.7MB dependency which isn't extensively used.