Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Settle on an interface for Futures.allAsList/successfulAsList (and Google-internal allAsMap/successfulAsMap) #1519

Open
gissuebot opened this issue Oct 31, 2014 · 6 comments
Labels
P3 package=concurrent status=research type=debeta Request to remove something from @Beta type=enhancement Make an existing feature better

Comments

@gissuebot
Copy link

gissuebot commented Oct 31, 2014

Original issue created by [email protected] on 2013-08-29 at 09:11 PM


  1. Are there better names? (Do they deserve the same name with disambiguating parameters? Do they at least deserve names that are near one another alphabetically?) Here's an internal-only doc with some discussion: https://docs.google.com/a/google.com/document/d/1Ry0Jx0tIJ8zFX9TnH6dbVCRGlYwrlkyiyekSYnay1VA/edit ("Possible rename of bulk Future methods")
  2. Would successfulAsList be better if it returned a list containing only the successes rather than a list containing interleaves successes and nulls? (Previously, people might have wanted to match up indexes (though IIRC my survey showed that few if any users were doing this). Nowadays, they can use successfulAsMap [edit: oops, currently Google-internal], which makes it easier to associate values with keys (which is probably more natural than associating them with indexes, anyway.) (Less plausible advantage: A user might really want to distinguish between "returned null" and "failed." This would let him do that, as the "failed" entries would go away.) Another option is to add a third method with the failures stripped, but I doubt we'd want to bother.
  3. When an allAsList input fails, should we cancel the other inputs? Should this be configurable? Here's an internal-only thread with some discussion: https://groups.google.com/a/google.com/d/topic/java-users/9utRs2xAafk/discussion

[edit: see also #3394 about a Collector-based equivalent]

@gissuebot
Copy link
Author

Original comment posted by [email protected] on 2014-08-13 at 01:49 PM


An internal user would like to be able to access the result as a Future<ImmutableList<V>> rather than a Future<PlainListThatHappensToBeImpossibleToMutate<V>>. He's using a framework that requires known immutable objects. He can transform(), of course, but if we make change (2), this would be an opportunity to switch to ImmutableList (at the cost of some verbosity to users who don't care).

An additional feature to consider is some kind of configuration of logging (or, more generally, of handling of failures). Our current policy can be a bit spammy, but we would fear dropping important messages if we turned logging off universally.

This still doesn't make the API clear. Future<ImmutableList<Foo>> f = Futures.collect(list).allMustSucceed(), etc.? There are many options.

@cpovirk
Copy link
Member

cpovirk commented Dec 9, 2016

We introduced Futures.{whenAllSucceed,whenAllComplete},{call,callAsync}. But we haven't addressed the List case, cancellation, or logging configuration.

@cpovirk
Copy link
Member

cpovirk commented Jan 11, 2017

Hacky workaround: Ensure that every failure exception thrown is equals() to every other failure exception. You may be able to accomplish that if you control the exception type that's thrown, or you can using Futures.catching() to effectively replace any failure with a custom exception.

@cpovirk cpovirk added the type=debeta Request to remove something from @Beta label Mar 12, 2019
@raghsriniv raghsriniv added the P3 label Aug 2, 2019
nick-someone pushed a commit that referenced this issue Mar 4, 2020
We can't store a plain V because we need to distinguish between Optional.absent() and null (at least in the Google-internal successfulAsMap -- and eventually in whenAllComplete(...).collectToList() (#1519), which is likely to omit failures instead of mapping them to null as successfulAsList does).

We may have problems using Optional when we adopt new nullability annotations, since Optional<V> might not be a valid instantiation when V is instantiated with a nullable type.

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=298398456
nick-someone pushed a commit that referenced this issue Mar 4, 2020
We can't store a plain V because we need to distinguish between Optional.absent() and null (at least in the Google-internal successfulAsMap -- and eventually in whenAllComplete(...).collectToList() (#1519), which is likely to omit failures instead of mapping them to null as successfulAsList does).

We may have problems using Optional when we adopt new nullability annotations, since Optional<V> might not be a valid instantiation when V is instantiated with a nullable type.

RELNOTES=n/a

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=298398456
@cpovirk
Copy link
Member

cpovirk commented Dec 1, 2020

See also "SEVERE: input future failed" spams stdout/stderr (#2134), in which I propose a possible workaround for people with that problem.

@eamonnmcmanus eamonnmcmanus self-assigned this Nov 3, 2021
copybara-service bot pushed a commit that referenced this issue Apr 11, 2023
Fixes #3403 #1519 #1421 #3424

RELNOTES=`concurrent`: Remove `@Beta` from `Futures`.
PiperOrigin-RevId: 523457329
@kluever
Copy link
Member

kluever commented Apr 11, 2023

This is now fixed.

@kluever kluever closed this as completed Apr 11, 2023
@cpovirk
Copy link
Member

cpovirk commented Apr 11, 2023

(Oops, I forgot about the logging business. Still, I'm not sure that the @Beta-ness of the API would make much practical difference in how disruptive such a change would be.)

I'm going to reopen this issue to track the theoretical possibility that we'd (a) introduce a better API for this (whenAllComplete().collectSuccessful() or whatever) and (b) introduce a Map version publicly.

@cpovirk cpovirk reopened this Apr 11, 2023
copybara-service bot pushed a commit that referenced this issue Apr 11, 2023
Fixes #3403 #1519 #1421 #3424

RELNOTES=`concurrent`: Remove `@Beta` from `Futures`.
PiperOrigin-RevId: 523465435
@eamonnmcmanus eamonnmcmanus removed their assignment Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 package=concurrent status=research type=debeta Request to remove something from @Beta type=enhancement Make an existing feature better
Projects
None yet
Development

No branches or pull requests

6 participants