Skip to content

Migrate from Yahoo DataSketches to Apache DataSketches 7.0.1 (KLL)#4774

Merged
merlimat merged 1 commit into
apache:masterfrom
lhotari:lh-update-datasketches
May 5, 2026
Merged

Migrate from Yahoo DataSketches to Apache DataSketches 7.0.1 (KLL)#4774
merlimat merged 1 commit into
apache:masterfrom
lhotari:lh-update-datasketches

Conversation

@lhotari

@lhotari lhotari commented May 5, 2026

Copy link
Copy Markdown
Member

Motivation

BookKeeper still depends on com.yahoo.datasketches:sketches-core:0.8.3, which has been unmaintained since the project moved to the Apache Software Foundation and was renamed to org.apache.datasketches:datasketches-java. A previous attempt to upgrade (#3264, May 2022) replaced the dependency 1:1 with the Apache classic-quantiles DoublesSketch but never landed.

Since #3264 two things have changed:

  1. BookKeeper master now requires Java 17, so we can use the latest datasketches-java:7.0.1 (which itself requires JDK ≥ 17).
  2. The Apache DataSketches maintainers explicitly recommend the KLL sketch over the classic quantiles sketch for any new code — see [UpdateDoublesSketch] More heap allocation than 0.8.3 datasketches-java#398 (comment). KLL is faster, more memory efficient, and has comparable accuracy at the default K. Migrating straight to KLL while we are touching this code avoids the near-term performance regression that Update datasketches version from 0.8.3 to 3.2.0 #3264 would have introduced.

Changes

  • pom.xml and stats/bookkeeper-stats-providers/prometheus-metrics-provider/pom.xml: dependency coordinate updated from com.yahoo.datasketches:sketches-core:0.8.3org.apache.datasketches:datasketches-java:7.0.1.
  • DataSketchesOpStatsLogger.java: ported to org.apache.datasketches.kll.KllDoublesSketch.
    • On-heap throughout, matching the previous behavior — no off-heap conversion.
    • KLL has no DoublesUnion; rotation aggregates with merge() directly into a freshly allocated KllDoublesSketch.newHeapInstance().
    • KLL has no reset(); per-thread sketches are reassigned to a fresh newHeapInstance() under the existing StampedLock write lock (the lock provides the happens-before edge that publishes the new reference safely).
    • KllDoublesSketch.getQuantile() throws SketchesArgumentException on an empty sketch (vs the classic sketch returning NaN), so getQuantileValue() now guards with isEmpty() to preserve the prior NaN return.
  • LICENSE-all.bin.txt / LICENSE-server.bin.txt: list datasketches-java-7.0.1.jar and the transitively-required datasketches-memory-4.1.0.jar separately. The two jars are sourced from distinct Apache repositories (apache/datasketches-java and apache/datasketches-memory), so each gets its own footnote — [25] retargeted to datasketches-java, and the previously-vacant [27] slot now references datasketches-memory.

The CVE-2021-40531 OWASP suppression that #3264 added does not apply to 7.0.x, so no suppression is needed.

Verification

  • mvn -pl stats/bookkeeper-stats-providers/prometheus-metrics-provider -am dependency:tree -Dincludes=org.apache.datasketches confirms the resolved versions: datasketches-java:jar:7.0.1:compiledatasketches-memory:jar:4.1.0:compile (matches the LICENSE entries).
  • mvn -pl stats/bookkeeper-stats-providers/prometheus-metrics-provider -am clean verify builds green; all 11 tests in TestPrometheusFormatter and TestPrometheusMetricsProvider pass.
  • mvn -pl stats/bookkeeper-stats-providers/prometheus-metrics-provider -am spotbugs:check -DskipTests: no errors/warnings.

The com.yahoo.datasketches:sketches-core:0.8.3 dependency has been
unmaintained since the project moved to Apache. Switch to the current
org.apache.datasketches:datasketches-java:7.0.1 and adopt the KLL
quantiles sketch, which the upstream maintainers recommend over the
classic DoublesSketch for new code (faster, smaller, comparable
accuracy at default K=200).

DataSketchesOpStatsLogger changes:
- On-heap throughout, matching the previous code (no off-heap usage).
- KLL has no reset(); per-thread sketches are reassigned to a fresh
  newHeapInstance() under the existing StampedLock write lock.
- KLL has no DoublesUnion; aggregation uses merge() directly into a
  freshly allocated KllDoublesSketch each rotation.
- KllDoublesSketch.getQuantile() throws SketchesArgumentException on
  an empty sketch, so getQuantileValue() now guards with isEmpty() to
  preserve the prior NaN return.

LICENSE bundles list datasketches-java-7.0.1.jar and the transitively
required datasketches-memory-4.1.0.jar separately, since the two jars
come from distinct upstream repositories.
@hezhangjian

Copy link
Copy Markdown
Member

I previously submitted a similar PR (#4774), which wasn’t merged due to performance concerns at the time. This new PR seems to revisit that approach, so it might be worth taking another look.

@merlimat @hangc0276 PTAL.

@lhotari

lhotari commented May 5, 2026

Copy link
Copy Markdown
Member Author

I previously submitted a similar PR (#4774), which wasn’t merged due to performance concerns at the time. This new PR seems to revisit that approach, so it might be worth taking another look.

@merlimat @hangc0276 PTAL.

@hezhangjian yes, I took your previous PR (#3264) into account.
The maintainers explained in this comment that KLL is faster, more memory efficient, and has comparable accuracy at the default K.

There's no reason to keep using the 10 year old dependency.

@merlimat merlimat merged commit ad8f4fc into apache:master May 5, 2026
20 checks passed
@hangc0276 hangc0276 added dependencies Pull requests that update a dependency file release/4.18.0 labels May 27, 2026
@hangc0276 hangc0276 added this to the 4.18.0 milestone May 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file release/4.18.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants