Repository navigation
chore(deps): remove guava from server - #2324
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The Caffeine cache lacks regression coverage, and the fractional-rate test does not exercise token refill behavior.
2 open findings
What changed in this PR
Removes the server’s direct Guava dependency, replacing its remaining usages with JDK, Caffeine, and Bucket4j alternatives.
Changes:
- Replaces Guava collection and stream utilities with JDK APIs.
- Reworks bounded stream reads and memoization.
- Migrates mirror request throttling to Bucket4j with evenly spaced calls.
| File | Description |
|---|---|
server/build.gradle |
Removes Guava dependency. |
server/gradle/libs.versions.toml |
Removes Guava catalog entries. |
server/CHANGELOG.md |
Records dependency and rate-limiter changes. |
server/src/main/java/org/eclipse/openvsx/ExtensionService.java |
Implements bounded package reads. |
server/src/main/java/org/eclipse/openvsx/RegistryAPI.java |
Replaces iterable duplicate checks. |
server/src/main/java/org/eclipse/openvsx/analytics/ingestion/DownloadIngestionMetrics.java |
Uses Caffeine for metric caching. |
server/src/main/java/org/eclipse/openvsx/analytics/ingestion/DownloadIngestionProcessor.java |
Implements JDK list partitioning. |
server/src/main/java/org/eclipse/openvsx/mirror/aop/UpstreamRegistryServiceAspect.java |
Migrates throttling to Bucket4j. |
server/src/main/java/org/eclipse/openvsx/repositories/DownloadAnalyticsRepository.java |
Implements batched sublists. |
server/src/main/java/org/eclipse/openvsx/util/DrainOnCloseInputStream.java |
Implements bounded drain loop. |
server/src/main/java/org/eclipse/openvsx/web/ValidationExceptionHandler.java |
Finds validation path leaves without Guava. |
server/src/test/java/org/eclipse/openvsx/mirror/aop/UpstreamRegistryServiceAspectTest.java |
Adds rate-limiter tests. |
server/src/test/java/org/eclipse/openvsx/util/DrainOnCloseInputStreamTest.java |
Updates bounded-drain coverage. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@cstamas I guess you are very happy with this change, however we need to make sure that this does not break any existing functionality. Also it might make sense to add useful replacements to our utils package like a limiting bytestream. |
|
Yup, Guava is creeping in everywhere while it should not, especially when on Java 25. Will take a peek. |
Replace the remaining Guava calls with JDK code, plus Caffeine and Bucket4j, which the server already depends on: - Lists.partition, Iterables, ByteStreams.limit: subList, streams and bounded read loops - Suppliers.memoizeWithExpiration: a Caffeine LoadingCache - RateLimiter: a Bucket4j blocking bucket that spaces mirror calls evenly; it no longer lets a burst through after an idle period Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Add CollectionUtil.partition and use it in DownloadIngestionProcessor and DownloadAnalyticsRepository instead of two hand-written subList loops. DrainOnCloseInputStream.close now drains with skipNBytes. Document the mirror rate limit change in the changelog: requests are now spaced evenly instead of allowing a burst after an idle period. Shorten two comments that described removed code.
|
Added one commit with a small cleanup. Also, tried to make guava scope runtime (to ensure nothing picks it up anymore), but Small fix to PR description: "Guava is still on the |
netomi
left a comment
netomi
left a comment
There was a problem hiding this comment.
Two points on the replacements, otherwise the swap looks equivalent to me.
1. The rate limiter is now interruptible (UpstreamRegistryServiceAspect)
bucket.consume(1) throws InterruptedException when the waiting thread is interrupted. Guava's RateLimiter.acquire() slept uninterruptibly and never threw. With the PR's bucket configuration on Bucket4j 8.10.1, an interrupt during the wait throws InterruptedException and leaves the interrupt flag cleared. The advice is declared throws Throwable, so the exception now escapes into the mirror code, and a caller that catches it generically loses the interrupt status.
BlockingBucket#consumeUninterruptibly(long) exists in 8.10.1 and keeps the previous behaviour. If the new behaviour is intended, it is worth a line in the changelog entry next to the even-spacing change.
For reference, I ran the same configuration standalone: 12 concurrent calls at 10 rps took about 1101 ms, and 3 calls after an idle period took about 200 ms, so the even spacing and the absence of a stored burst match the description.
2. DrainOnCloseInputStream#close no longer updates bytesRead
skipNBytes goes through FilterInputStream#skip, which delegates to the wrapped stream and bypasses read, so bytesRead is not advanced by the drain. This matters because close() appears to run twice on this stream: once from the try-with-resources in LocalRegistryService and once from try (var input = content) in createExtensionFile. The second close skips again on an already-closed stream. The resulting IOException is swallowed, so it works in practice, but close() is no longer a no-op the second time, and a runtime exception from a closed stream would not be caught. I did not confirm that LocalRegistryService passes the same stream into createExtensionFile.
Reading through this (for example readNBytes into a discarded buffer, or a small loop on read(byte[])) would keep bytesRead accurate and make the second close cheap, as before.
…oseInputStream.close idempotent
The guava removal and the mirror rate limit change were merged after v1.3.0 was tagged, but renaming [next] to [1.3.0] listed them under the release. Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Removes the direct
com.google.guava:guavadependency from the server. Guava was used in 8 files, and every call now uses the JDK or a library the server already depends on.Lists.partitionsubListloopsIterables.limit/Iterables.anystream().limit(previousSize).noneMatch(...)Iterables.getLast(Path)ByteStreams.limitExtensionServiceandDrainOnCloseInputStreamSuppliers.memoizeWithExpirationLoadingCacheRateLimiterBehaviour change: the mirror rate limit (
ovsx.data.mirror.requests-per-second) now spaces calls evenly. Guava let up to one second of requests through at once after an idle period.-1still means unlimited, and 0 or less (or NaN) still fails at startup.Guava is still on the runtime classpath through
google-cloud-storage.