[FEATURE] Change synchronous Transport Request call to async for extension initialization

[owaiskazi19]

Is your feature request related to a problem?

Current Transport Request calls to OpenSearch are synchronous, a better way will be to make them asynchronous if in them future we want to make parallel requests.

The ExtensionsInitRequestHandler makes four calls in its handleExtensionInitRequest method:

extensionsRunner.sendRegisterRestActionsRequest(extensionTransportService);
extensionsRunner.sendRegisterCustomSettingsRequest(extensionTransportService);
extensionsRunner.transportActions.sendRegisterTransportActionsRequest(
    extensionTransportService,
    extensionsRunner.opensearchNode,
    extensionsRunner.getUniqueId()
);
// Get OpenSearch Settings and set values on ExtensionsRunner
Settings settings = extensionsRunner.sendEnvironmentSettingsRequest(extensionTransportService);

The first three calls currently execute asynchronously but there is no checking whether they are acknowledged before the extension is considered initialized.

The last call to get the settings executes asynchronously but blocks with a countdown latch.

What solution would you like?

Have all four send*() methods return a CompletableFuture.

• The first three "register" methods would return a CompletableFuture<AcknowledgedResponse>
  • the AcknowledgedResponseHandler should complete / completeExceptionally this future
• The settings request would return a CompletableFuture<Settings>.
  • the EnvironmentSettingsResponseHandler should complete / completeExceptionally this future, replacing the current countdown latch / await completion setup

Create an .allOf() completable future that waits (orTimeout(....).join()) for all the other requests to complete before continuing on with the rest of the init method.

[minalsha]

Can you share the status on this @owaiskazi19 ?

[dbwiddis]

Most of the original request has been handled via other issues. Updated the initial comment to describe the one remaning task to do, (a)synchronizing the initialization sequence.

[vyvy3]

Wanna take the issue and have a question: when the error occurs during register request execution, should the ExtensionsRunner manually throw an exception so AcknowledgedResponseHandler will handle it, or is it primarily based on the status that lies inside AcknowledgedResponse? Are there any examples in the repository where I can see similar functionality to make it more straightforward for me?

[dbwiddis]

when the error occurs during register request execution, should the ExtensionsRunner manually throw an exception so AcknowledgedResponseHandler will handle it, or is it primarily based on the status that lies inside AcknowledgedResponse?

You'll need to pass a CompletableFuture to the AcknowledgedResponseHandler (you can create a new constructor with that Future as a parameter). Then in the handleResponse() block you'll complete the future; in the handleException() block you'll complete the future exceptionally. So some very rough pseudocode:

• in each sendFooRequest() method (most in ExtensionsRunner but not all), Instantiate a CompletableFuture
• Instantiate an AcknowledgedResponseHandler for each request using that future as a parameter
• change the return of those methods from void to returning the future
• in the ExtensionsInitRequestHandler collect all of those futures and wait for all of them to complete

Are there any examples in the repository where I can see similar functionality to make it more straightforward for me?

I'm not sure we have done this other than anonymous subclasses like here but the concept should be similar (just imagine the future as a parameter internal to the handler) that gets returned.

[dbwiddis]

Hey @qchimera are you actively working on this? No worries if you are and need a bit more time, just let me know.

[vyvy3]

@dbwiddis yeah I'm still working on this

[vyvy3]

One more addition, sendEnvironmentSettingsRequest currently working with the Settings class, when all the methods of EnvironmentSettingsResponseHandler are shaped to work with CompletableFuture<EnvironmentSettingsResponse>, should I create a new handler for this? Or duplicate methods so they will work directly with Settings class? @dbwiddis

The rest looks to be done, only this part stops from debugging the code

[dbwiddis]

Great question.

When we wrote that method we essentially made it synchronous, so it served as a natural stopping point for the initialization. It's your choice whether to rewrite it to be asynchronous the same as everything else (probably my preference) or to leave it as is.

Note that it must complete before the code using its result.

[vyvy3]

It's your choice whether to rewrite it to be asynchronous the same as everything else (probably my preference)

This means, going deeply into detail, I still need to work with EnvironmentSettingsResponse class, but separate future-based operation from the EnvironmentSettingsResponseHandler to the root (ExtensionsInitRequestHandler in this case), where all the futures are collected. Only then, after collecting the response, I will take out an instance of Settings and then use it, again doing everything in the root class, correct?

I re-read it and it sounds horrible, hope you'll understand me xD

[dbwiddis]

I re-read it and it sounds horrible, hope you'll understand me xD

Based on that disclaimer I skipped reading to independently see what needed to be done, let's see if it matches yours:

• you'll be removing the awaitResponse() code and directly returning the CompletableFuture<EnvironmentSettingsResponse> to the init request handler.
• You'll add that one to the allOf() call which must complete before extracting the settings from that future to use in the follow-on code.

Now I read your description and that looks like what you said! :)

[vyvy3]

Well I screwed a bit and had pushed my changes also with #512, since it was my main branch, can you check it urself if possible, I'll for now wait until you will merge my changes just so make it easier.

Or can I completely clean my main branch? just thinking

[dbwiddis]

I'm still debugging something on #512 so don't expect it until this weekend.

I suggest you do a few things:

• stop using your main branch for features. 😁 You're not the only one to make mistakes like this. Always get a fresh copy of main and create a new branch to start working, and rebase with main often.
• to fix this problem use interactive git rebase. DO NOT DO THIS ON MAIN. Create a branch (git checkout -b newbranchnamehere).
  • git rebase -i HEAD~9 will show you the last 9 commits (not sure if that's too few or too many). Delete the commits not associated with the current PR. Then you can push this and create a new PR.

GIven that your open PR from main is blocking you from properly using git, I'll prioritize getting that merged soon, probably this weekend.