Replacing CompletableFuture by CompletableResultCode#2670
Replacing CompletableFuture by CompletableResultCode#2670trask merged 3 commits intoopen-telemetry:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the disk-buffering module to avoid java.util.concurrent.CompletableFuture (not available on older Android API levels and not desugared) by switching async result handling to OpenTelemetry SDK’s CompletableResultCode.
Changes:
- Replaced
CompletableFuture<WriteResult>withCompletableResultCodeinSignalStorageand all implementations/callers. - Removed
WriteResult/DefaultWriteResulttypes and updated storage/exporter logic accordingly. - Updated tests and renamed exporter timeout constants to align with “write” semantics.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| disk-buffering/src/test/java/io/opentelemetry/contrib/disk/buffering/internal/exporters/SignalStorageExporterTest.java | Updates tests to reflect CompletableResultCode-based storage results. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/storage/result/WriteResult.java | Removes legacy write result abstraction (no longer needed). |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/storage/result/DefaultWriteResult.java | Removes legacy WriteResult implementation. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/storage/impl/FileSpanStorage.java | Updates storage API to return CompletableResultCode. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/storage/impl/FileMetricStorage.java | Updates storage API to return CompletableResultCode. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/storage/impl/FileLogRecordStorage.java | Updates storage API to return CompletableResultCode. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/storage/SignalStorage.java | Public API change: write/clear now return CompletableResultCode. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/internal/storage/FileSignalStorage.java | Updates write/clear implementations to produce CompletableResultCode outcomes. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/internal/exporters/SignalStorageExporter.java | Switches exporter write handling from Future.get(timeout) to CompletableResultCode.join(timeout). |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/exporters/SpanToDiskExporter.java | Renames default timeout constant to DEFAULT_WRITE_TIMEOUT. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/exporters/MetricToDiskExporter.java | Renames default timeout constant to DEFAULT_WRITE_TIMEOUT. |
| disk-buffering/src/main/java/io/opentelemetry/contrib/disk/buffering/exporters/LogRecordToDiskExporter.java | Renames default timeout constant to DEFAULT_WRITE_TIMEOUT. |
Comments suppressed due to low confidence (1)
disk-buffering/src/test/java/io/opentelemetry/contrib/disk/buffering/internal/exporters/SignalStorageExporterTest.java:83
- This test suite doesn’t cover the timeout path anymore. Since
exportToStoragenow relies onCompletableResultCode.join(timeout, unit), it’s important to assert behavior when the returnedCompletableResultCodedoes not complete before the timeout (e.g., storage returns a new, never-completedCompletableResultCode). Add a test that verifiesonExportErrorreceives a timeout-related throwable and that the returned result code reflects that timeout.
@Test
void verifyExportToStorage_failure() {
SignalStorage.Span storage = mock();
SignalStorageExporter<SpanData> storageExporter =
new SignalStorageExporter<>(storage, callback, Duration.ofSeconds(1));
SpanData item1 = mock();
// Without exception
when(storage.write(anyCollection())).thenReturn(CompletableResultCode.ofFailure());
List<SpanData> items = Collections.singletonList(item1);
CompletableResultCode resultCode = storageExporter.exportToStorage(items);
| public interface SignalStorage<T> extends Iterable<Collection<T>>, Closeable { | ||
|
|
||
| /** | ||
| * Stores signal items. | ||
| * | ||
| * @param items The items to be stored. | ||
| * @return A future with {@link WriteResult}. | ||
| * @return A {@link CompletableResultCode} representing the outcome of the write operation. | ||
| */ | ||
| CompletableFuture<WriteResult> write(Collection<T> items); | ||
| CompletableResultCode write(Collection<T> items); | ||
|
|
||
| /** | ||
| * Removes all the previously stored items. | ||
| * | ||
| * @return A future with {@link WriteResult}. | ||
| * @return A {@link CompletableResultCode} representing the outcome of the clear operation. | ||
| */ | ||
| CompletableFuture<WriteResult> clear(); | ||
| CompletableResultCode clear(); |
There was a problem hiding this comment.
Changing SignalStorage.write/clear from CompletableFuture<WriteResult> to CompletableResultCode is a breaking change to a public API. If this module follows semver/stability guarantees, consider providing a migration path (e.g., keeping the old methods as deprecated default methods for one release, or documenting the breaking change in the appropriate changelog/release notes) so downstream implementers aren’t broken unexpectedly.
There was a problem hiding this comment.
Given that disk-buffering is still not marked as stable, this is a feasible breaking change. It won't affect users who rely on the provided [Signal]ToDiskExporter implementations from this lib, and even if some users were manually accessing the storage APIs, the CompletableFuture capabilities have a 1:1 replacement in CompletableResultCode, as the latter was created as an alternative for CompletableFuture in mind.
| public CompletableResultCode exportToStorage(Collection<T> items) { | ||
| CompletableFuture<WriteResult> future = storage.write(items); | ||
| try { | ||
| WriteResult operation = future.get(writeTimeout.toMillis(), MILLISECONDS); | ||
| if (operation.isSuccessful()) { | ||
| callback.onExportSuccess(items); | ||
| return CompletableResultCode.ofSuccess(); | ||
| } | ||
| CompletableResultCode result = | ||
| storage.write(items).join(writeTimeout.toMillis(), TimeUnit.MILLISECONDS); | ||
| if (result.isSuccess()) { | ||
| callback.onExportSuccess(items); |
There was a problem hiding this comment.
CompletableResultCode.join(timeout, unit) can return even when the result is not completed (see other usages that check isDone() after join). In that case this code will treat an incomplete result as a plain failure with null throwable, which changes the previous behavior where timeouts produced an exceptional failure (and onExportError received the timeout). Consider checking result.isDone() after join(...) and, if false, invoking onExportError with a TimeoutException (and returning CompletableResultCode.ofExceptionalFailure(...)). Also consider preserving/propagating thread interruption if the join returns early due to interrupt.
bidetofevil
left a comment
bidetofevil
left a comment
There was a problem hiding this comment.
Looks good to me. The return logic in SignalStorageExporter could probably be simplified to improve readability (there are 4 return statements in which only one produces a successful result, but it's sandwiched between 2 blocks that return failures), but that shouldn't block it if you don't have time.
Thanks. I've updated it to improve readability. |
java.util.concurrent.CompletableFutureis only available from Android API 24+. Android'sdesugar_jdk_libsdoes not desugarCompletableFuture, which causes this issue: open-toast/gummy-bears#168This was confirmed by reproducing the crash on an API 21 emulator.
To avoid this issue, I replaced all
CompletableFutureusage withCompletableResultCodefrom the upstream Java SDK.