Add NativeExecutionProcess and facilities

[miaoever]

• Add the NativeExecutionProcess class to be responsible to launch the native process.
• Add the health check of the process launching by calling the native process's /v1/info endpoint with backoff retry.

[v-jizhang]

@bot kick off tests

[tanjialiang]

Why do we need this new class? It does not seem to have additional functionality compared to its parent

[miaoever]

The reason is I want to introduce the any native execution concept into the RequestErrorTracker in the presto-main module.

[tanjialiang]

I don't quite understand. Can you clarify?

[miaoever]

Removed the PrestoSparkRequestErrorTracker and use the RequestErrorTracker directly.

[tanjialiang]

If we choose to use the RequestErrorTracker to handle failure, it will be nice to also change result fetch and info fetch to use the same, to be consistent.

[tanjialiang]

Or we probably won't be needing that fancy backoff logic in the tracker at all. That IMO works better in a multi-tenant environment when resource contention is a commonly happening scenario. It could just be simple logic of n times retry with interval of m seconds. Up to you.

[tanjialiang]

This method does not seem to be used publicly, or more than one place. Shall we remove this method, or change it to private?

[miaoever]

Moved the retry method out to the caller, so keep this method public for the caller.

[tanjialiang]

The retry logic should not be inside of http client class. Can we move it to the caller class? Or somewhere better

[tanjialiang]

Yeah so is this one. I think this one calls the below private method. client class should only deal with simple http calls, sync or async.

[tanjialiang]

Are we trying to use port to compose the path?

[miaoever]

Yes, we're locating the config file with the presto_cpp binary in the same folder (to leverage the file cleanup mechanism), the binary path will be shared by all the containers running on the same host, so use the port number in the path to isolate the config for different tasks.

[tanjialiang]

I thought we had isolated file systems for each container?

[tanjialiang]

Would be nice if we can have the commands configurable through. That way we can enable/disable any additional features dynamically.

[miaoever]

Sounds good, I can add that in my next PR.

[tanjialiang]

Should we throw presto exception?

[tanjialiang]

Why do we put this here? It does not seem like been used.

[miaoever]

it will be used by caller (e.g native operator) by calling public NativeExecutionTask getTask()

[arunthirupathi]

This field can be marked as final.

[tanjialiang]

Why do we want to combine the factories? They seem logically separated. NativeExecutionProcess is responsible for launching the process and maintain health, more like a worker role. NativeExecutionTask is responsible for scheduling task and retrieving results from external process, more like a coordinator role.

[miaoever]

Agreed with what you said but I think that's the exact reason we have separated classes for NativeExecutionProcess and NativeExecutionTask, but for the factory class, IMO the factory is used to control the creation of the object, in our case, the NativeExecutionProcess should naturally be responsible to create the NativeExecutionTask since the NativeExecutionTask can only work after the process launch/initialization having been finished.

[tanjialiang]

Initially we created this factory as a workaround to take in injected objects from GUICE framework where we did't actually need a factory for factory purpose for NativeExecutionTask. But since we used the factory pattern it will be better to follow the pattern convention: XXXFactory shall create XXX. Here AAAFactory 1) creates 2 different types AAA and BBB, 2) is stateful and has control plan logic (stop()). It looks more like some mix of creation and control.

[tanjialiang]

We can have a separate NativeExecutionProcessFactory (it's okay to have 2 factories) that only creates a NativeExecutionProcess (for the purpose of accepting GUICE injections) and let operator do the rest (lifecycle management etc).

[tanjialiang]

One more question: Do we need to take care of the shutdown of injected resources? Shouldn't they be managed by the framework?

[miaoever]

Updated to bring back the NativeExecutionTaskFactory

[tanjialiang]

Thanks @miaoever for working on this. Left some initial comments.
It would also be nice to break it down to multiple smaller commits so that it is easier to review.

[arunthirupathi]

In Presto the convention is.

static final

final

non final fields.

[arunthirupathi]

This field can be marked as final.

[arunthirupathi]

This is assigned but always overwritten, is this assignment required ?

[miaoever]

You're right, it's not required, just removed.

[arunthirupathi]

others are calling shutdownNow, this calls shutdown

[miaoever]

good catch.

[arunthirupathi]

Should we log the errors when it fails to start ?

[arunthirupathi]

this method can be static.

[arunthirupathi]

this method can be static.

[arunthirupathi]

This method is bound to have race conditions, we are opening and closing the port, and other thread could re-open the same port. Not sure if my understanding is right.

[miaoever]

Once we close the socket with a given port, the TCP will be in the TIME_WAIT states for certain time (minutes IIRC), so during the TIME_WAIT period, that port won't be selected again by other processes/threads. Although this can not avoid the race condition totally, it can largely reduce the likelihood IMO. Without a central coordinated mechanism (say the coordinator assigns unique ports to different workers), this probably the only feasible way to choose an available port by each worker independently. Happy to hear your thought here.

[arunthirupathi]

Do we want to log the failedReason as well here ?

[arunthirupathi]

should this method be marked as @ Override

[arunthirupathi]

nit: It is generally an anti pattern to overwrite the config property in the code.

Can there be multiple workers on the same box, if so there is a chance for all the workers to keep mutating this property.

[miaoever]

This code will be executed on the worker side, so the mutation is only visible to the current worker IIUC. The reason we have to pick and assign the port per worker is in our prod environment, there is no port isolation among all the containers running on the same host, so we have to pick unique port per worker to avoid port collision. This system config (NativeExecutionSystemConfig) will be passed down to the presto_cpp process eventually.

[arunthirupathi]

Can you please add this as a comment on why we are doing this ?

[arunthirupathi]

Ensure that this property can never be overriden and only be set by the system admins. Otherwise, bad actors might trick Presto in to running untrusted binaries.

It will also be good, if you ensure that the file exists with this executable and fail with a meaningful error when the file is not present.

[miaoever]

That's a good point. On Sapphire/Spark side (internally) we prevent the bad actor by not distributing untrusted libraries, so even user set this property, the library won't be distributed to the worker for execution.

We also already have the file not exist check inside the getProcessWorkingPath method.

[arunthirupathi]

Some more comments, looks good.

[arunthirupathi]

This will NPE, if the object is created, but not started and then closed.

[arunthirupathi]

Can you please add this as a comment on why we are doing this ?

[arunthirupathi]

Since this will be use visible also wrap this in PrestoException.

[arunthirupathi]

The method returns void, but this is retrieved and ignored. Can you please add comment around the requirement of the code.

[tanjialiang]

I don't quite understand. Can you clarify?

[tanjialiang]

NativeExecutionTask is not used in the class other than a getter. Could we move NativeExecutionTask to be owned by the same entity that owns NativeExecutionProcess?

[miaoever]

I think since logically the NativeExecutionProcess need to outlast the NativeExecutionTask and IMO it's reasonable to let it controls the lifecycle of the NativeExecutionTask, WDYT?

[tanjialiang]

Since we are also referencing NativeExecutionTask in NativeExecutionOperator for calls and controls then operator would be a better place for both task and process. I believe putting task inside process does not actually do lifecycle control as JVM has its own garbage collection mechanism. Even if last reference to process is gone it will just be the process that is garbage collected but not the task within as long as there is something referencing that task.

Also if we think about higher level concepts, a class being a member is a belong-to relation. Here a task exists more like in parallel with process in a way that process is an abstract concept of cpp process, and it communicates with task through http. Together they get a job done.
If we compare it to the universal "car" example:

• native process is the car engine.
• native task is the car tire.
• native operator is the car.
  having task inside process is like having tire inside engine. But car also references tire as it needs it to roll and run. Now it creates this coupling that is generally discouraged if it could be avoided.

I'm approving this change to unblock since it's been a while. It will be the best to make the change in this PR but I'm okay if we want to do it in a follow-up.

[miaoever]

IMO, practically NativeExecutionTask strictly depends on the lifecycle of NativeExecutionProcess, if the process has died or not started yet, the task shouldn't be called by the user, giving both the NativeExecutionTask and NativeExecutionProcess to the user (e.g NativeExecutionOperator) to make sure every time they want to call the task , the native process is alive/available is not ideal - Ideally, we should expose the NativeExecutionTask's APIs through the NativeExecutionProcess and check the process's status before actually calling into the NativeExecutionTask. I can do a it in the follow-up diff.

[tanjialiang]

I see Presto code base other places doing the same thing (shutting down resources in factory class, basing on the @PreDestroy annotation). It'll do the job but just super weird place. Let's keep it this way then.

[tanjialiang]

Maybe we just need one class for http calls. Can we merge this one and PrestoSparkHttpWorkerClient? Maybe not in this PR. We can put it as a todo.

[miaoever]

Added the TODO as suggested.

[tanjialiang]

Thanks MJ, overall looks good. Just some nits

[tanjialiang]

Approve to unblock. Please make sure to read the comments before proceeding.

[tanjialiang]

Approve to unblock. Please make sure to read the comments before proceeding.

TODOs as discussed:

1. move NativeExecutionTask out from NativeExecutionProcess (if not done in this PR).
2. merge task/server clients to a single one.
3. Change info fetcher and result fetcher to use RequestErrorTracker to be consistent.