-
Notifications
You must be signed in to change notification settings - Fork 172
Description
Component(s)
disk-buffering
Is your feature request related to a problem? Please describe.
I find the design of StorageIterator counterintuitive, because as opposed to most Iterators, its [next() method](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/Iterator.html#next() has a side-effect, namely that it deletes a file from the disk. I would even say that it violates the "contract" of the Iterator.next() method, because according to its javadoc it is only supposed to return the next element, so changes to the underlying data structure are unexpectable.
In the end I think the counterintuitive design contributed to the provided example in the README and the implementation in the opentelemetry-android project being logically incorrect (#2539 & open-telemetry/opentelemetry-android#1493).
Describe the solution you'd like
I would find an API for the SignalStorage more fool-proof, that requires the explicit notation of successful or unsuccessful consumption of the batches.
Describe alternatives you've considered
e.g.
Alternative 1:
spanStorage.consume((batch) -> {
CompletableResultCode resultCode = networkExporter.export(spanData);
resultCode.join(timeout, TimeUnit.MILLISECONDS);
return resultCode.isSuccess(); // if the return value is false, the file won't be deleted and optionally the iteration is aborted
});Alternative 2:
for (Iterator<Collection<SpanData>> iterator = spanStorage.iterator(); iterator.hasNext(); ) {
Collection<SpanData> spanData = iterator.next();
CompletableResultCode resultCode = networkExporter.export(spanData);
resultCode.join(timeout, TimeUnit.MILLISECONDS);
if (!resultCode.isSuccess()) {
// The iteration should be aborted here to avoid consuming batches, which were not exported successfully
iterator.remove(); // .remove() is called explicitly
}
}```
Alternative 2 leaves more flexibility to the user, because the user can decide to continue or to abort the iteration. However, it would certainly surprise devs just bumping the version of the disk buffering lib without inserting the `remove()` logic into the code.
LMK what you think, @LikeTheSalad, @breedx-splk
### Additional context
- #2084
- #2183
### Tip
<sub>[React](https://github.blog/news-insights/product-news/add-reactions-to-pull-requests-issues-and-comments/) with 👍 to help prioritize this issue. Please use comments to provide useful context, avoiding `+1` or `me too`, to help us triage it. Learn more [here](https://opentelemetry.io/community/end-user/issue-participation/).</sub>