> On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote: > > src/checks/checker.hpp > > Lines 69-70 (original), 73-74 (patched) > > <https://reviews.apache.org/r/64386/diff/8/?file=1941021#file1941021line73> > > > > These guys can now be part of `MesosRuntimeInfo` : )
And fix the spelling in the comment tweak :) s/containierizer/containerizer/ > On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote: > > src/checks/checker_process.hpp > > Lines 40-47 (patched) > > <https://reviews.apache.org/r/64386/diff/8/?file=1941023#file1941023line40> > > > > What meaning do you put in the difference between `UCR` and `MESOS`? > > For me, the difference is whether the checker library talks to the task > > directly or via a container runtime; as a consequence, different types of > > command checks is used. The `UCR` and `MESOS` enums should be combined for now, as there is no logical difference between them at the moment. (You could actually use nested container health checks on "`MESOS`" executors, if you had the necessary credentials.) > On Jan. 22, 2018, 5:27 a.m., Alexander Rukletsov wrote: > > src/checks/checker_process.hpp > > Lines 40-69 (patched) > > <https://reviews.apache.org/r/64386/diff/8/?file=1941023#file1941023line40> > > > > These types remind me of protobufs. I do not envision a need to make > > them protobufs in the nearest future. Here is what comes to my mind instead: > > ``` > > class RuntimeOptions > > { > > public: > > enum class Type > > { > > MESOS, > > DOCKER, > > UCR > > }; > > > > private: > > const Type type; > > }; > > > > class MesosRuntimeOptions: public RuntimeOptions > > { > > public: > > MesosRuntimeOptions(...): RuntimeOptions(MESOS), ...; > > > > private: > > const Option<pid_t> taskPid; > > const std::vector<std::string> namespaces; > > }; > > > > class DockerRuntimeOptions: public RuntimeOptions > > { > > public: > > DockerRuntimeOptions(...): RuntimeOptions(Docker), ...; > > > > private: > > const std::string dockerPath; > > const std::string socketName; > > const std::string containerName; > > } > > ``` > > What do you think? I would also suggest to declare these types in a > > separate file to minimize dependencies and hence compilation time. Also, > > this should likely be a separate patch ; ) I broadly agree with this refactor... Subclassing the `RuntimeOptions` means that you'll either need: (A) The `CheckerProcess::commandCheck()` method to up-cast to `DockerRuntimeOptions`. If we put other information inside these classes (like `taskPid` for the `MesosRuntimeOptions`), then there will be lots of up-casting all over the Health checker code. (B) Or we will need to add a virtual method to `RuntimeOptions` to hold the logic inside `wrapDockerExec`. And other virtual methods as necessary to indirectly expose the other private fields. I'm inclined towards (B). Note: `taskPid` and `namespaces` are used by the Docker executor's health checks too. The `docker exec` is run inside the same `net` namespace as the executor's PID. So there aren't many options specific to the `MesosRuntimeOptions` at the moment. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64386/#review195894 ----------------------------------------------------------- On Jan. 17, 2018, 4:10 a.m., Akash Gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64386/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2018, 4:10 a.m.) > > > Review request for mesos, Alexander Rukletsov, Andrew Schwartzmeyer, Gaston > Kleiman, Jie Yu, Joseph Wu, and Michael Park. > > > Repository: mesos > > > Description > ------- > > Added information about the calling executor to the health check > library. After the refactoring, the command health check code originally > in the docker executor is now in the health check library. > > > Diffs > ----- > > src/checks/checker.hpp 93502270f31e80c5f7c94b5b456625e9cdea1837 > src/checks/checker.cpp fff0aac504b4283a210f936e00c977fa60d88b3d > src/checks/checker_process.hpp 510f3b2e6e689faaf26595214ce377c2b5518f28 > src/checks/checker_process.cpp ddb197b8cc2c503fef5ae20af32f5881fff9833f > src/checks/health_checker.hpp 019fbd791f250ecc28ff59d779f90e7ccbf0c685 > src/checks/health_checker.cpp eaf9a18817eeeff7c29c7a4b9d1b183f398760a3 > src/docker/docker.hpp d9e71f8841a868082170d28fc4f3d495e2eb1e61 > src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a > src/launcher/default_executor.cpp 4a619859095cc2d30f4806813f64a2e48c83b3ea > src/launcher/executor.cpp 050f5a057f360873e2b4738b126289bcd1bd0c7f > > > Diff: https://reviews.apache.org/r/64386/diff/8/ > > > Testing > ------- > > See https://reviews.apache.org/r/64387/ > > > Thanks, > > Akash Gupta > >
