Add future to be notified when async init finishes.

[jkozlowski]

General

Before this PR:
In order to either block until initialization is done OR be notified when it is ready, users need to poll #isInitialized, wasting a thread.

After this PR:

==COMMIT_MSG==
Add future to be notified when async init finishes.
==COMMIT_MSG==

Priority:

P1

Concerns / possible downsides (what feedback would you like?):

• I have replaced manual state tracking with just a future (which more accurately models what is happening, and reuses things.
• it's unclear if refactoring an interface for async initialization is a good call at this stage, however this code is rather intertwined together.

Is documentation needed?:

Compatibility

Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?:

• #isInitialized was moved to a subinterface.

Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?:

The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.):

Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?:

Does this PR need a schema migration?

Testing and Correctness

What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?:

What was existing testing like? What have you done to improve it?:

If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.:

• This code now contains somewhat annoying level of concurrency. We have AsyncInitializer which is used by initialization code to synchronize it's uses. Then user's can get access to the internal future, which should track the state.
• Synchronization in AsyncInitializer is used to ensure that the initializer does the right thing.
• So I think the implementation is still correct.

If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?:

Execution

How would I tell this PR works in production? (Metrics, logs, etc.):

Has the safety of all log arguments been decided correctly?:

Will this change significantly affect our spending on metrics or logs?:

How would I tell that this PR does not work in production? (monitors, etc.):

If this PR does not work as expected, how do I fix that state? Would rollback be straightforward?:

If the above plan is more complex than “recall and rollback”, please tag the support PoC here (if it is the end of the week, tag both the current and next PoC):

Scale

Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.:

Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?:

Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?:

Development Process

Where should we start reviewing?:

If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?:

Please tag any other people who should be aware of this PR:
@jeremyk-91
@sverma30
@raiju

[changelog-app]

Generate changelog in changelog/@unreleased

What do the change types mean?

• feature: A new feature of the service.
• improvement: An incremental improvement in the functionality or operation of the service.
• fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
• break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
  and external consumers of the service's API (e.g. customer-written software or integrations).
• deprecation: Advertises the intention to remove service functionality without any change to the
  operation of the service itself.
• manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
  performing database surgery, ...) at the time of upgrade for it to succeed.
• migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

• ❗The break and manual task changelog types will result in a major release!
• 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
• ✨ All others will result in a minor version release.

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Add future to be notified when async init finishes.

Check the box to generate changelog(s)

• Generate changelog entry

[jeremyk-91]

Yeah this looks reasonable - main thing is a question on the API of AsyncInitializing. The implementation is as far as I can tell right!

[jeremyk-91]

For this: we can probably use futures on the assert directly? The output'll be a bit nicer if it actually fails

[jeremyk-91]

oof this one is a bit weirder. A bit funky, but I think assertThat(actual.isInitializedAsync()).satisfiesAnyOf(FutureAssert::isNotDone, FutureAssert::isCancelled) expresses what you want here?

[jeremyk-91]

For this one, I think I understand what you're trying to do, but we should discuss to make sure, and I think we might want to name it slightly different - maybe getInitializationFuture or similar. isInitializedAsync to me at least suggests "compute isInitialized in the background, and give me the result when it's done" which I don't think is what you intend.

Assuming the above is right, I can see two ways we can take this:

• if we don't want to expose the known-initialized version of the object to listeners once computation is done, we can probably just return a ListenableFuture<Void> unless I'm missing something
• if we do want to, should this return T and have the interface take a type parameter T as well?

[jkozlowski]

You make a good point. I was thinking about just "give me something to wait on until the object is initialized. Your second API would kind of make sense to me if we didn't expose the objects already, which we do all over the place.

As in the design of async init in Atlas is that you always get access to the underlying resource, BUT it throws exceptions until it's initialized.

To me ListenableFuture<?> actually carries this intent: it's just a notification thing. Lemme play around with actually returning <T> maybe that's actually what we want here.

[jkozlowski]

Alright so returning T is a much bigger effort, I think. The current decomp is such that Wrappers extend AsyncInitializer and they kind of just use it as a state tracker/state machine that they drive. So nowhere is T available.

However, I guess tryInitialize could return T, lemme try that.

[jkozlowski]

So #tryInitialize returning T, means that AsyncInitializable needs to have a generic parameter, so then everything that implements it needs to provide that. Which starts spiriling, and ultimately I don't feel like that's the right decomp.

We should RETURN AsyncInitializable<KeyValueService> from factories, not have KeyValueService extend that, I think this is my pet annoyance with the decomp here.

Lemme see how destructive this would be to just do for KeyValueService to prototype this cleanup (I'll have AtlasDbFactory force returning a wrapper, not the type itself). This is much closer to how Witchcraft returns Client interfaces from ConjureFactory and I think that's the right model here.

[jkozlowski]

Ultimately it feels like async init in atlas is just a false dichotomy as it's implemented: user's can choose to async initialize or not. But instead we should async initialize by default (because presumably it's faster since you can init unrelated pieces concurrently, instead of serially), but users should have a choice of whether they want to throw and block until it's done or let the rest of their system init.

[jkozlowski]

And I see we tried to do graphs of components, but it feels like in a rather clunky way, tieing initialization to the actual objects which pollutes the interfaces. And ultimately it just doesn't model this very well, because even though you're kind of building a graph, all the resources still need to handle closing their subcomponents correctly and correctly implementing the #isInitialized check and it's all just a bit silly.

[jkozlowski]

Alright, having said all that what I'm proposing is obviously very complicated and totally beside the scope of this PR. I will rename the method to #getInitializationFuture() likely and call it a day and save this rambling for another day.