Adapter pattern #142
Adapter pattern #142
Conversation
basert
force-pushed
the
adapter-pattern
branch
13 times, most recently
from
e90bc74 to
29e16c8
Compare
| // Wait for runtime | ||
| for ($i = 0; $i < 10; $i++) { | ||
| $output = ''; | ||
| $code = Console::execute('docker container inspect ' . \escapeshellarg($runtimeName), '', $output); |
There was a problem hiding this comment.
strange I thought we used the Docker API here? Doesn't the Docker() adapter have this method?
There was a problem hiding this comment.
This is copied from: https://github.com/open-runtimes/executor/blob/main/app/http.php#L409
| * @param Log $log | ||
| * @return mixed | ||
| */ | ||
| abstract public function createExecution( |
There was a problem hiding this comment.
whats the difference between createRuntime and createExecution? Are they not the same?
There was a problem hiding this comment.
createRuntime is called by:
Http::post('/v1/runtimes')
->desc("Create a new runtime server")
createExecution is called by:
Http::post('/v1/runtimes/:runtimeId/executions')
->alias('/v1/runtimes/:runtimeId/execution')
->desc('Create an execution')
A runtime can be used by the execution, e.g. in Kubernetes it calls itself internally. Here I just moved the http.php code into different functions.
| case Storage::DEVICE_LOCAL: | ||
| default: | ||
| return new Local($root); | ||
| case Storage::DEVICE_S3: |
There was a problem hiding this comment.
would be a good chance to migrate this to DSN format like we do for databases. Will also make it easier to handle any new adapters and maintenance in general.
src/Executor/Runner/Docker.php
Outdated
| * @param Table $statsContainers | ||
| * @return void | ||
| */ | ||
| public function init(Table $activeRuntimes, array $networks, Table $statsHost, Table $statsContainers): void |
There was a problem hiding this comment.
Will we need to handle state like this also in the other adapter? If we do, can we have a parent class that handles this on a higher level? This will help us make adapters leaner and avoid having to pass this knowledge/tasks between adapters.
Can we also move this logic to the constructor instead of having an init method?
src/Executor/Runner/Docker.php
Outdated
| * @param Table $statsContainers | ||
| * @return void | ||
| */ | ||
| public function init(Table $activeRuntimes, array $networks, Table $statsHost, Table $statsContainers): void |
There was a problem hiding this comment.
Another ideas is to maybe place the state management on the server level and this way isolate all the swoole related stuff from this layer. It will make upgrading server version easier, especially with the planned move to the new coroutine version in a few weeks/months.
basert
force-pushed
the
adapter-pattern
branch
5 times, most recently
from
ae70b10 to
6370516
Compare
basert
force-pushed
the
adapter-pattern
branch
from
6370516 to
0c913eb
Compare
This moves runner related code into an adapter.