Re: Review Request 73004: Documented setting offer constraints via the scheduler API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73004/#review222166 --- Ship it! Ship It! - Vinod Kone On Nov. 4, 2020, 7:56 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/73004/ > --- > > (Updated Nov. 4, 2020, 7:56 p.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-10193 > https://issues.apache.org/jira/browse/MESOS-10193 > > > Repository: mesos > > > Description > --- > > Documented setting offer constraints via the scheduler API. > > > Diffs > - > > docs/app-framework-development-guide.md > 8f30045b04d1a8f3c0d078ff79e65ba9c1e7c94a > docs/scheduler-http-api.md e447ffb43253fd927bdb266ad12a4554c69e0ee8 > > > Diff: https://reviews.apache.org/r/73004/diff/1/ > > > Testing > --- > > Tested manually how Github renders this: > https://github.com/asekretenko/mesos/blob/offer_constraints_doc/docs/app-framework-development-guide.md > https://github.com/asekretenko/mesos/blob/offer_constraints_doc/docs/scheduler-http-api.md > > > Thanks, > > Andrei Sekretenko > >
Review Request 72862: Added cleanup of build dir to website bot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72862/ --- Review request for mesos and Andrei Sekretenko. Repository: mesos Description --- Added cleanup of build dir to website bot. Diffs - support/mesos-website/entrypoint.sh 72fd72375e63f7e0dddb5472187edfdbe7a9d148 Diff: https://reviews.apache.org/r/72862/diff/1/ Testing --- Tested via https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/16/console Thanks, Vinod Kone
Review Request 72861: Added `--no-same-owner` option to tar command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72861/ --- Review request for mesos and Andrei Sekretenko. Repository: mesos Description --- This ensures extracted tarballs are owned by root when running the build as root. Diffs - 3rdparty/Makefile.am 23f49ecffc5b44529ce69ccd841d942a30a98c2d Diff: https://reviews.apache.org/r/72861/diff/1/ Testing --- Thanks, Vinod Kone
Review Request 72860: Added `--no-same-owner` option to tar command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72860/ --- Review request for mesos and Andrei Sekretenko. Repository: mesos Description --- This ensures extracted tarballs are owned by root when running the build as root. Diffs - 3rdparty/libprocess/3rdparty/Makefile.am e148f2dd2470ba973cc3f6d4067be64039299a48 Diff: https://reviews.apache.org/r/72860/diff/1/ Testing --- Thanks, Vinod Kone
Review Request 72859: Added `--no-same-owner` option to tar command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72859/ --- Review request for mesos and Andrei Sekretenko. Repository: mesos Description --- This ensures extracted tarballs are owned by root when running the build as root. Diffs - 3rdparty/stout/3rdparty/Makefile.am 5a90a11c67131f21dc4487222ea1dde228dd Diff: https://reviews.apache.org/r/72859/diff/1/ Testing --- Thanks, Vinod Kone
Re: Review Request 72856: Fixed Website bot to work with docker user namespaces.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72856/ --- (Updated Sept. 10, 2020, 9:36 p.m.) Review request for mesos, Andrei Sekretenko, Benjamin Mahler, and Gilbert Song. Changes --- Added a check for file ownership. Repository: mesos Description --- ASF CI has enabled user namespaces for docker daemon on the build machines that are used to build the website. This commit makes the requisite changes to work with user namespacing. Diffs (updated) - support/mesos-website.sh 04a664611cb882813e18d8b45da4aabb578e7d44 support/mesos-website/Dockerfile 611c4962d1eb99b2e8ea225bb5dcf840d973ab9c support/mesos-website/build.sh afbec486e892b580348091dc790da651ff4b5952 support/mesos-website/entrypoint.sh 72fd72375e63f7e0dddb5472187edfdbe7a9d148 Diff: https://reviews.apache.org/r/72856/diff/3/ Changes: https://reviews.apache.org/r/72856/diff/2-3/ Testing --- Tested via https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/4/console Thanks, Vinod Kone
Re: Review Request 72858: Reduced build parallelism to avoid aborted builds in CI.
> On Sept. 10, 2020, 1:54 p.m., Andrei Sekretenko wrote: > > support/mesos-website/entrypoint.sh > > Line 35 (original), 35 (patched) > > <https://reviews.apache.org/r/72858/diff/1/?file=2239413#file2239413line35> > > > > Maybe leave a comment that higher values lead to stuck/killed builds in > > the ASF CI, most likely due to out of memory conditions? done - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72858/#review221846 --- On Sept. 10, 2020, 12:14 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72858/ > --- > > (Updated Sept. 10, 2020, 12:14 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Reduced build parallelism to avoid aborted builds in CI. > > > Diffs > - > > support/mesos-website/entrypoint.sh > 72fd72375e63f7e0dddb5472187edfdbe7a9d148 > > > Diff: https://reviews.apache.org/r/72858/diff/1/ > > > Testing > --- > > https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/4/console > > > Thanks, > > Vinod Kone > >
Review Request 72858: Reduced build parallelism to avoid aborted builds in CI.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72858/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Repository: mesos Description --- Reduced build parallelism to avoid aborted builds in CI. Diffs - support/mesos-website/entrypoint.sh 72fd72375e63f7e0dddb5472187edfdbe7a9d148 Diff: https://reviews.apache.org/r/72858/diff/1/ Testing --- https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/4/console Thanks, Vinod Kone
Review Request 72857: Merged build.sh and entrypoint.sh.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72857/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Repository: mesos Description --- Merged build.sh and entrypoint.sh. Diffs - support/mesos-website/build.sh afbec486e892b580348091dc790da651ff4b5952 support/mesos-website/entrypoint.sh 72fd72375e63f7e0dddb5472187edfdbe7a9d148 Diff: https://reviews.apache.org/r/72857/diff/1/ Testing --- https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/4/console Thanks, Vinod Kone
Review Request 72856: Fixed Website bot to work with docker user namespaces.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72856/ --- Review request for mesos, Andrei Sekretenko, Benjamin Mahler, and Gilbert Song. Repository: mesos Description --- ASF CI has enabled user namespaces for docker daemon on the build machines that are used to build the website. This commit makes the requisite changes to work with user namespacing. Debug. Fix. Diffs - support/mesos-website.sh 04a664611cb882813e18d8b45da4aabb578e7d44 support/mesos-website/Dockerfile 611c4962d1eb99b2e8ea225bb5dcf840d973ab9c support/mesos-website/entrypoint.sh 72fd72375e63f7e0dddb5472187edfdbe7a9d148 Diff: https://reviews.apache.org/r/72856/diff/1/ Testing --- Tested via https://ci-builds.apache.org/job/Mesos/job/Mesos-Websitebot-test/4/console Thanks, Vinod Kone
Re: Review Request 72413: Updated executor API docs to include the domain socket.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72413/#review220425 --- Fix it, then Ship it! docs/executor-http-api.md Lines 76 (patched) <https://reviews.apache.org/r/72413/#comment308812> Is the location also sent to the executor via an environment variable? If yes, we should specify the name of it. If not, I think that would make it hard to write a domain socket communicating executor? - Vinod Kone On April 22, 2020, 5:44 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72413/ > --- > > (Updated April 22, 2020, 5:44 a.m.) > > > Review request for mesos, Benjamin Mahler and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Updated executor API docs to include the domain socket. > > > Diffs > - > > docs/executor-http-api.md 4af4cd444dbeb29c115c46444dc01da0e86fd848 > > > Diff: https://reviews.apache.org/r/72413/diff/1/ > > > Testing > --- > > Viewed with `site/mesos-website-dev.sh`. > > > Thanks, > > Greg Mann > >
Review Request 72196: Removed stale MPI framework.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72196/ --- Review request for mesos, Benjamin Bannier and Benjamin Mahler. Bugs: MESOS-6084 https://issues.apache.org/jira/browse/MESOS-6084 Repository: mesos Description --- The framework code hasn't been touched in years and likely doesn't work anymore. Diffs - Makefile.am 11192acd54109990edeee4451eccb3ac4af382ea configure.ac 4aa4f7612db287d8424f92dbcb4936d458c83198 mpi/README 691ae78c413b9e4050381e17457ece01ac1bfa18 mpi/mpiexec-mesos.in dc0695339dedb3e208d91340503764aa466c95f5 mpi/mpiexec-mesos.py 932e2f183b97a66cd84c0676ab84bd87c4f2e1bc Diff: https://reviews.apache.org/r/72196/diff/1/ Testing --- Tested in internal CI. Thanks, Vinod Kone
Re: Review Request 72055: Changed logic of termination of the Docker executor.
> On Jan. 29, 2020, 10:28 a.m., Qian Zhang wrote: > > src/docker/executor.cpp > > Lines 786-787 (original) > > <https://reviews.apache.org/r/72055/diff/1/?file=2209872#file2209872line786> > > > > We have a fail safe in command executor: > > https://github.com/apache/mesos/blob/1.9.0/src/launcher/executor.cpp#L1060:L1062 > > , do we want do the similar in Docker executor to ensure it can still self > > terminate in case the agent doesn't send an ACK for the terminal update for > > some reason? let's add the fail safe please. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72055/#review219410 --- On Jan. 28, 2020, 2:13 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72055/ > --- > > (Updated Jan. 28, 2020, 2:13 p.m.) > > > Review request for mesos, Andrei Sekretenko, Greg Mann, Qian Zhang, and Vinod > Kone. > > > Bugs: MESOS-9847 > https://issues.apache.org/jira/browse/MESOS-9847 > > > Repository: mesos > > > Description > --- > > Previously, the Docker executor terminated itself after a task's > container had terminated. This could lead to termination of the > executor before receiving all status update acknowledgments from > the agent. In order to mitigate this issue, the executor slept for > one second to give a chance to send all status updates and receive > all status update acknowledgments before terminating itself. This > might have led to various race conditions in some circumstances > (e.g., on a slow host). This patch terminates the Docker executor > after receiving a terminal status update acknowledgment. Also, > this patch removes the unnecessary call of sleep. > > > Diffs > - > > src/docker/executor.cpp 132f42bfa42c846fc5dc40f7763aa0b5d12a7798 > src/exec/exec.cpp 69e5e24b248c7c913421de5e42713c34fd79ad46 > > > Diff: https://reviews.apache.org/r/72055/diff/1/ > > > Testing > --- > > internal CI > > > Thanks, > > Andrei Budnik > >
Re: Review Request 71652: Avoid double reaping race in the command executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71652/#review218354 --- Ship it! Ship It! - Vinod Kone On Oct. 23, 2019, 3:52 a.m., Benjamin Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71652/ > --- > > (Updated Oct. 23, 2019, 3:52 a.m.) > > > Review request for mesos, Gilbert Song, Greg Mann, Qian Zhang, and Vinod Kone. > > > Bugs: MESOS-10007 > https://issues.apache.org/jira/browse/MESOS-10007 > > > Repository: mesos > > > Description > --- > > Previously, it was possible for the command executor to miss > the exit status of the subprocess since it performs its own > reap and doesn't use the Subprocess::status. See MESOS-10007. > > > Diffs > - > > src/launcher/executor.cpp d9b39e5a66eb7bc739dede52f43da8ca9f7c09d1 > > > Diff: https://reviews.apache.org/r/71652/diff/1/ > > > Testing > --- > > make check + manual testing from Charles in JIRA > > > Thanks, > > Benjamin Mahler > >
Re: Review Request 71559: Added early exit in reviewbot if nothing to review.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71559/#review218009 --- Ship it! Ship It! - Vinod Kone On Sept. 30, 2019, 1:06 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71559/ > --- > > (Updated Sept. 30, 2019, 1:06 p.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > --- > > The current reviewbot setup always makes sure that the `master` branch > can be built before verifying any reviews. This takes considerable time. > > With this patch we now avoid any builds if no reviews need to be > verified. While detecting reviews requiring verification takes some > time, it is currently much faster than building Mesos. With this setup > we should now finish this job much faster in the usual case (nothing > needs verification) while taking only on the order of a minute longer > otherwise. > > > Diffs > - > > support/jenkins/reviewbot.sh c5b497634cb9754fa19c5af3f579ceae25ab912d > > > Diff: https://reviews.apache.org/r/71559/diff/1/ > > > Testing > --- > > Verified early exit by running locally. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71497/#review217792 --- drive by review. 3rdparty/libprocess/src/openssl.cpp Lines 118 (patched) <https://reviews.apache.org/r/71497/#comment305180> can you use the `alias` argument in `add()` for these? docs/ssl.md Lines 100 (patched) <https://reviews.apache.org/r/71497/#comment305179> These deprecations need to be documented in CHANGELOG and upgrades.md. - Vinod Kone On Sept. 17, 2019, 12:50 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71497/ > --- > > (Updated Sept. 17, 2019, 12:50 p.m.) > > > Review request for mesos, Greg Mann and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to > `LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`. > > The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to > `LIBPROCESS_SSL_VERIFY_SERVER_CERT`. > > The new names better describe the actual effect of both flags, and > make upgrades easier by allowing operators to only enable verification > on agents that are new enough to contain the updated hostname > validation code paths. > > > Diffs > - > > 3rdparty/libprocess/include/process/ssl/flags.hpp > 1a0e3820cc8cd1459625f46a54b194133500f11e > 3rdparty/libprocess/src/openssl.hpp > 271cc95238d287c06df36478554502e8b7205b09 > 3rdparty/libprocess/src/openssl.cpp > 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 > 3rdparty/libprocess/src/tests/ssl_tests.cpp > 9d5ab679165a709f7c3740020961ec89a7db4f54 > docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 > > > Diff: https://reviews.apache.org/r/71497/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71415: Updated `release-guide.md`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71415/#review217513 --- Ship it! Ship It! - Vinod Kone On Aug. 30, 2019, 1:10 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71415/ > --- > > (Updated Aug. 30, 2019, 1:10 p.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Removed unnecessary steps and added the steps for pushing changes to > remote master and release branch. > > > Diffs > - > > docs/release-guide.md ce9c7c1605a47bf0d9babcf899c0128a0169d901 > > > Diff: https://reviews.apache.org/r/71415/diff/1/ > > > Testing > --- > > Non-code change. > > > Thanks, > > Qian Zhang > >
Review Request 71394: Skipped GPG signing for snapshot builds.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71394/ --- Review request for mesos, Benjamin Bannier and Till Toenshoff. Bugs: MESOS-9955 https://issues.apache.org/jira/browse/MESOS-9955 Repository: mesos Description --- Skipped GPG signing for snapshot builds. Diffs - support/snapshot.sh c30aca02ec999320e2b3f0f3ce36c04aa0425ae3 Diff: https://reviews.apache.org/r/71394/diff/1/ Testing --- https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console Thanks, Vinod Kone
Review Request 71395: Implemented snapshot bot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71395/ --- Review request for mesos, Benjamin Bannier and Till Toenshoff. Bugs: MESOS-9955 https://issues.apache.org/jira/browse/MESOS-9955 Repository: mesos Description --- Dockerized the snapshot building process. Diffs - support/jenkins/snapshotbot.sh PRE-CREATION support/mesos-snapshot.sh PRE-CREATION Diff: https://reviews.apache.org/r/71395/diff/1/ Testing --- https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console Thanks, Vinod Kone
Review Request 71393: Skiped GPG signing during `maven-install` in Makefile.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71393/ --- Review request for mesos, Benjamin Bannier and Till Toenshoff. Bugs: MESOS-9955 https://issues.apache.org/jira/browse/MESOS-9955 Repository: mesos Description --- GPG signing is not needed for snapshot JARs (since they are typically done by CI). But our snapshot building workflow, i.e., support/snapshot.sh, relies on `make maven-install` which forces GPG signing to happen. This allows our snapshot script to skip GPG signing by doing `mvn deploy -Dgpg.skip`. Note that GPG signing still happens for RCs and release JARs (which are typically done by humans) when `mvn deploy` (without `-Dgpg.skip`) is called (e.g., in support/vote.sh). Diffs - src/Makefile.am 5daee632d52e882acc15b90f655a53dab23eaaf6 Diff: https://reviews.apache.org/r/71393/diff/1/ Testing --- https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console Thanks, Vinod Kone
Review Request 71392: Improved snapshot script to deduce the Mesos version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71392/ --- Review request for mesos. Bugs: MESOS-9955 https://issues.apache.org/jira/browse/MESOS-9955 Repository: mesos Description --- Previously the script expected the version to be provided as an argument. Diffs - support/snapshot.sh c30aca02ec999320e2b3f0f3ce36c04aa0425ae3 Diff: https://reviews.apache.org/r/71392/diff/1/ Testing --- https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console Thanks, Vinod Kone
Review Request 71391: Fixed sed arg to work on Linux and OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71391/ --- Review request for mesos. Bugs: MESOS-9955 https://issues.apache.org/jira/browse/MESOS-9955 Repository: mesos Description --- Fixed sed arg to work on Linux and OSX. Diffs - support/snapshot.sh c30aca02ec999320e2b3f0f3ce36c04aa0425ae3 Diff: https://reviews.apache.org/r/71391/diff/1/ Testing --- https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console Thanks, Vinod Kone
Review Request 71390: Fixed the snapshot script to be non-interactive.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71390/ --- Review request for mesos, Benjamin Bannier and Till Toenshoff. Bugs: MESOS-9955 https://issues.apache.org/jira/browse/MESOS-9955 Repository: mesos Description --- Fixed the snapshot script to be non-interactive. Diffs - support/snapshot.sh c30aca02ec999320e2b3f0f3ce36c04aa0425ae3 Diff: https://reviews.apache.org/r/71390/diff/1/ Testing --- https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console Thanks, Vinod Kone
Re: Review Request 71389: Added CHANGELOG entry for agent draining.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71389/#review217480 --- Ship it! Ship It! - Vinod Kone On Aug. 28, 2019, 2:24 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71389/ > --- > > (Updated Aug. 28, 2019, 2:24 p.m.) > > > Review request for mesos, Greg Mann, Joseph Wu, and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > Added CHANGELOG entry for agent draining. > > > Diffs > - > > CHANGELOG 935463849cc937e82d01254624ca10a11b9ad5ee > docs/upgrades.md 63eb1bbe401d4fb36e11e595a6b1954793265b33 > > > Diff: https://reviews.apache.org/r/71389/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71375: Added quota limits to the 1.9.0 CHANGELOG.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71375/#review217454 --- Ship it! Ship It! - Vinod Kone On Aug. 27, 2019, 4:56 p.m., Benjamin Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71375/ > --- > > (Updated Aug. 27, 2019, 4:56 p.m.) > > > Review request for mesos, Andrei Sekretenko, Meng Zhu, Qian Zhang, and Vinod > Kone. > > > Repository: mesos > > > Description > --- > > Added quota limits to the 1.9.0 CHANGELOG. > > > Diffs > - > > CHANGELOG 8ce9ee6a5f55b37765d53c5735d20c74be5ce8e2 > > > Diff: https://reviews.apache.org/r/71375/diff/1/ > > > Testing > --- > > > Thanks, > > Benjamin Mahler > >
Re: Review Request 71309: Added more details to the Bintray release guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71309/#review217294 --- Ship it! Ship It! - Vinod Kone On Aug. 19, 2019, 5:03 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71309/ > --- > > (Updated Aug. 19, 2019, 5:03 p.m.) > > > Review request for mesos, Greg Mann, Joseph Wu, Till Toenshoff, and Vinod > Kone. > > > Repository: mesos > > > Description > --- > > Added more details to the Bintray release guide. > > > Diffs > - > > docs/release-guide.md 88281d691210307a1a6d6c5a9fd71f1607982694 > > > Diff: https://reviews.apache.org/r/71309/diff/1/ > > > Testing > --- > > Uploaded 1.8.1 package to bintray according to these instructions. > > > Thanks, > > Benno Evers > >
Re: Review Request 71244: Included task group's resources in the ExecutorInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71244/#review217125 --- Can you add a test for this? src/slave/slave.cpp Lines 3120 (patched) <https://reviews.apache.org/r/71244/#comment304392> s/taskResources/tasksResources/ - Vinod Kone On Aug. 7, 2019, 11:17 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71244/ > --- > > (Updated Aug. 7, 2019, 11:17 p.m.) > > > Review request for mesos, Andrei Budnik and Gilbert Song. > > > Bugs: MESOS-9925 > https://issues.apache.org/jira/browse/MESOS-9925 > > > Repository: mesos > > > Description > --- > > Included task group's resources in the ExecutorInfo. > > > Diffs > - > > src/slave/slave.cpp 74eb45744d6603b91676e812ed008a7b1ab39a49 > > > Diff: https://reviews.apache.org/r/71244/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Qian Zhang > >
Re: Review Request 71229: Fixed the webui roles table resource sorting.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71229/#review217041 --- Ship it! Ship It! - Vinod Kone On Aug. 1, 2019, 7:14 p.m., Benjamin Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71229/ > --- > > (Updated Aug. 1, 2019, 7:14 p.m.) > > > Review request for mesos, Andrei Sekretenko and Meng Zhu. > > > Repository: mesos > > > Description > --- > > All of the roles table resource columns were using the wrong > keys for sorting. > > > Diffs > - > > src/webui/app/roles/roles.html b290587463a6ebc17bf645530247e46d4e3cff2a > > > Diff: https://reviews.apache.org/r/71229/diff/1/ > > > Testing > --- > > manual testing > > > Thanks, > > Benjamin Mahler > >
Re: Review Request 71138: Updated configure.ac to correct openssl/libevent setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71138/#review216793 --- Ship it! Ship It! - Vinod Kone On July 22, 2019, 10:17 p.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71138/ > --- > > (Updated July 22, 2019, 10:17 p.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Fixes a problem introduced by configure.ac reordering in commit 4df2b62. > > > Diffs > - > > configure.ac 0e3058c2bcd730a0992ca497809ab09c58ed6fa1 > > > Diff: https://reviews.apache.org/r/71138/diff/1/ > > > Testing > --- > > Tested within DC/OS build CI which failed without this fix. > > > Thanks, > > Till Toenshoff > >
Re: Review Request 70528: Updated release guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70528/#review216485 --- Ship it! - Vinod Kone On July 10, 2019, 2:58 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70528/ > --- > > (Updated July 10, 2019, 2:58 p.m.) > > > Review request for mesos, Greg Mann, Joseph Wu, and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > Updated the release guide in `docs/release-guide.md` to match > the de-facto process we're using to release new versions. > > Also mentioned some additional third-party tooling to the post-release > desription. > > > Diffs > - > > docs/release-guide.md a3ad2668a1953a7f20dd7209e122481ad8b30f17 > > > Diff: https://reviews.apache.org/r/70528/diff/3/ > > > Testing > --- > > Released Mesos 1.8.0 according to this procedure. > > > Thanks, > > Benno Evers > >
Re: Review Request 70528: Updated release guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70528/#review216486 --- Ship it! Ship It! - Vinod Kone On July 10, 2019, 2:58 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70528/ > --- > > (Updated July 10, 2019, 2:58 p.m.) > > > Review request for mesos, Greg Mann, Joseph Wu, and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > Updated the release guide in `docs/release-guide.md` to match > the de-facto process we're using to release new versions. > > Also mentioned some additional third-party tooling to the post-release > desription. > > > Diffs > - > > docs/release-guide.md a3ad2668a1953a7f20dd7209e122481ad8b30f17 > > > Diff: https://reviews.apache.org/r/70528/diff/3/ > > > Testing > --- > > Released Mesos 1.8.0 according to this procedure. > > > Thanks, > > Benno Evers > >
Re: Review Request 70527: Updated Bintray URL in docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70527/#review216484 --- Ship it! Ship It! - Vinod Kone On April 23, 2019, 2:32 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70527/ > --- > > (Updated April 23, 2019, 2:32 p.m.) > > > Review request for mesos, Greg Mann and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > Updated Bintray URL in docs. > > > Diffs > - > > docs/binary-packages.md 462e732246aa3f3a69f243e512fa73012341 > > > Diff: https://reviews.apache.org/r/70527/diff/1/ > > > Testing > --- > > Opened the modified links in a browser to ensure no typos. > > > Thanks, > > Benno Evers > >
Re: Review Request 70959: Cleared agent drain state when draining is finished.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70959/#review216201 --- src/slave/slave.cpp Lines 4876 (patched) <https://reviews.apache.org/r/70959/#comment303285> there are several places in this file where `removeFramework` is called, why is this only being called here? i think it would be less error prone and future proof to call this from within `removeFramework()` ? src/slave/slave.cpp Lines 9791-9801 (patched) <https://reviews.apache.org/r/70959/#comment303284> can this be replaced with `return framework->idle()`? - Vinod Kone On June 27, 2019, 3:24 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70959/ > --- > > (Updated June 27, 2019, 3:24 p.m.) > > > Review request for mesos, Greg Mann and Joseph Wu. > > > Bugs: MESOS-9860 > https://issues.apache.org/jira/browse/MESOS-9860 > > > Repository: mesos > > > Description > --- > > Once a draining agent has neither frameworks with pending tasks nor any > executors with either queued or launched tasks it has finished draining. > This patch adds handling of that case which clears both the in-memory > and persisted drain configuration. > > > Diffs > - > > src/slave/slave.hpp 6954f53ff1531b9fcb688ef76acddf6a3d849a41 > src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e > > > Diff: https://reviews.apache.org/r/70959/diff/2/ > > > Testing > --- > > `make check` > > tested as part of https://reviews.apache.org/r/70960/ > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70898: Fixed flaky UpdateFrameworkV0Test.DriverErrorOnFrameworkIDMismatch.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70898/#review215977 --- Ship it! Ship It! - Vinod Kone On June 19, 2019, 6:37 p.m., Andrei Sekretenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70898/ > --- > > (Updated June 19, 2019, 6:37 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-9793 > https://issues.apache.org/jira/browse/MESOS-9793 > > > Repository: mesos > > > Description > --- > > This patch removes a spurious call of driver.start() before setting > expectations on the scheduler's mock methods. > > > Diffs > - > > src/tests/master/update_framework_tests.cpp > 3ac62c61db90eee626e59fbb09554f983d96f391 > > > Diff: https://reviews.apache.org/r/70898/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Sekretenko > >
Re: Review Request 70822: Added common protobufs for agent draining.
> On June 14, 2019, 9:34 a.m., Benjamin Bannier wrote: > > src/messages/messages.proto > > Lines 986 (patched) > > <https://reviews.apache.org/r/70822/diff/3/?file=2148591#file2148591line986> > > > > What does it mean if the master sends a `DRAINED` state to the agent? > > Is that something we need to reject in validation? > > > > Does it maybe make sense to instead break out `DrainInfo.state` into > > its own message to use in state reporting? > > Greg Mann wrote: > Yes we can have a CHECK on the agent to make sure the master doesn't send > DRAINED in a DrainSlaveMessage. That will happen in another patch. > > What's the benefit of moving `DrainInfo.State` outside of `DrainInfo`? > > Benjamin Bannier wrote: > The goal would be to use dedicated messages for triggering draining > (master->agent) and to report draining (agent->master). That might not only > be conceptually simplier, but would likely also lead to simpler, less > redundant code. Is there any benefit in using the same message? > > Greg Mann wrote: > What do you mean by "report draining"? The design doesn't involve any > explicit communication of draining progress between agent and master, the > master just receives task status updates. > > Benjamin Bannier wrote: > Sorry, there's no process of communicating `DrainInfo` back to master, > but we do report it to users with https://reviews.apache.org/r/70835/. > > Greg Mann wrote: > Are you proposing having a separate message for inclusion in the agent > API outputs? > > Vinod Kone wrote: > IIUC, Benjamin is questioning why we are storing both the spec (max grace > period, mark_gone) and the status (state) in the same proto (do we do this > elsewhere in the code base?). If they were separate protos, say DrainRequest > and DrainStatus, master would send DrainRequest to the agent, and show > DrainStatus (and possibly DrainRequest) in the operator API response. It > looks a bit weird that we would send DrainInfo (as it is currently written > to the agent and do a CHECK on valid state. The fact that an agent got a > Drain message is enough for the agent to know that it needs to drain, it > doesn't need to look into the `DrainInfo::State`? Let me repharse my first sentence. I think storing both spec and status in the same proto is probably fine if it makes thing easy to expose in API endpoints and use it internally in the master. But I still think it's weird to send the status in a message to the agent which is not expected to use it. So, if there's a way we can avoid it that would be great. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70822/#review215896 --- On June 18, 2019, 11:17 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70822/ > --- > > (Updated June 18, 2019, 11:17 p.m.) > > > Review request for mesos, Benjamin Bannier, Benno Evers, Benjamin Mahler, > Joseph Wu, and Vinod Kone. > > > Bugs: MESOS-9753 > https://issues.apache.org/jira/browse/MESOS-9753 > > > Repository: mesos > > > Description > --- > > This patch makes protobuf message updates which will be used > by both the master and the agent to facilitate automatic > draining of agents. > > > Diffs > - > > include/mesos/mesos.proto 2b4f350815935220c2d2b0dd0e52346bc74c91d9 > include/mesos/type_utils.hpp 57b1893160dbe874aa9fec00a3d1b640b9c54906 > include/mesos/v1/mesos.proto bafc27499f810791700c4a30dcb1da33b6f31d2e > src/common/type_utils.cpp ef1b3ea15cde1c7a8e0735fb9d7566dd1fd2cfdb > src/internal/devolve.hpp fefe86e450fa5083b9ff50e92f4594ffb30a54c8 > src/internal/devolve.cpp 1d300b49d5cc3de4b8ed409902eb881c7afc07ea > src/internal/evolve.hpp 1044d9df75b6fc1f60d3704be9cb5751e6d4321d > src/internal/evolve.cpp 19c155967bf090fb2ec39211805ff1385787ab59 > src/messages/messages.proto e30ad34cc9212b05f85ba5e1d4fcfc9e49ae92c0 > > > Diff: https://reviews.apache.org/r/70822/diff/5/ > > > Testing > --- > > `make` > > > Thanks, > > Greg Mann > >
Re: Review Request 70822: Added common protobufs for agent draining.
> On June 14, 2019, 9:34 a.m., Benjamin Bannier wrote: > > src/messages/messages.proto > > Lines 986 (patched) > > <https://reviews.apache.org/r/70822/diff/3/?file=2148591#file2148591line986> > > > > What does it mean if the master sends a `DRAINED` state to the agent? > > Is that something we need to reject in validation? > > > > Does it maybe make sense to instead break out `DrainInfo.state` into > > its own message to use in state reporting? > > Greg Mann wrote: > Yes we can have a CHECK on the agent to make sure the master doesn't send > DRAINED in a DrainSlaveMessage. That will happen in another patch. > > What's the benefit of moving `DrainInfo.State` outside of `DrainInfo`? > > Benjamin Bannier wrote: > The goal would be to use dedicated messages for triggering draining > (master->agent) and to report draining (agent->master). That might not only > be conceptually simplier, but would likely also lead to simpler, less > redundant code. Is there any benefit in using the same message? > > Greg Mann wrote: > What do you mean by "report draining"? The design doesn't involve any > explicit communication of draining progress between agent and master, the > master just receives task status updates. > > Benjamin Bannier wrote: > Sorry, there's no process of communicating `DrainInfo` back to master, > but we do report it to users with https://reviews.apache.org/r/70835/. > > Greg Mann wrote: > Are you proposing having a separate message for inclusion in the agent > API outputs? IIUC, Benjamin is questioning why we are storing both the spec (max grace period, mark_gone) and the status (state) in the same proto (do we do this elsewhere in the code base?). If they were separate protos, say DrainRequest and DrainStatus, master would send DrainRequest to the agent, and show DrainStatus (and possibly DrainRequest) in the operator API response. It looks a bit weird that we would send DrainInfo (as it is currently written to the agent and do a CHECK on valid state. The fact that an agent got a Drain message is enough for the agent to know that it needs to drain, it doesn't need to look into the `DrainInfo::State`? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70822/#review215896 --- On June 18, 2019, 11:17 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70822/ > --- > > (Updated June 18, 2019, 11:17 p.m.) > > > Review request for mesos, Benjamin Bannier, Benno Evers, Benjamin Mahler, > Joseph Wu, and Vinod Kone. > > > Bugs: MESOS-9753 > https://issues.apache.org/jira/browse/MESOS-9753 > > > Repository: mesos > > > Description > --- > > This patch makes protobuf message updates which will be used > by both the master and the agent to facilitate automatic > draining of agents. > > > Diffs > - > > include/mesos/mesos.proto 2b4f350815935220c2d2b0dd0e52346bc74c91d9 > include/mesos/type_utils.hpp 57b1893160dbe874aa9fec00a3d1b640b9c54906 > include/mesos/v1/mesos.proto bafc27499f810791700c4a30dcb1da33b6f31d2e > src/common/type_utils.cpp ef1b3ea15cde1c7a8e0735fb9d7566dd1fd2cfdb > src/internal/devolve.hpp fefe86e450fa5083b9ff50e92f4594ffb30a54c8 > src/internal/devolve.cpp 1d300b49d5cc3de4b8ed409902eb881c7afc07ea > src/internal/evolve.hpp 1044d9df75b6fc1f60d3704be9cb5751e6d4321d > src/internal/evolve.cpp 19c155967bf090fb2ec39211805ff1385787ab59 > src/messages/messages.proto e30ad34cc9212b05f85ba5e1d4fcfc9e49ae92c0 > > > Diff: https://reviews.apache.org/r/70822/diff/5/ > > > Testing > --- > > `make` > > > Thanks, > > Greg Mann > >
Re: Review Request 70567: Allowed compiling Seccomp isolator on older kernel versions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70567/#review214963 --- Fix it, then Ship it! LGTM src/slave/containerizer/mesos/isolators/linux/seccomp.cpp Lines 35 (patched) <https://reviews.apache.org/r/70567/#comment301226> s/the/The/ - Vinod Kone On April 29, 2019, 6:39 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70567/ > --- > > (Updated April 29, 2019, 6:39 p.m.) > > > Review request for mesos, Benno Evers, Gilbert Song, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > This patch removes dependency on `linux/seccomp.h` header, which > may be missing on some Linux distributions. > > > Diffs > - > > configure.ac e5afde4fcae320facd4a7cf2961999b238812b4b > src/slave/containerizer/mesos/isolators/linux/seccomp.cpp > 5624c24fc08f0eeb5cccff9cfb2fe22eef4bfb50 > > > Diff: https://reviews.apache.org/r/70567/diff/1/ > > > Testing > --- > > internal CI > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70546: WIP: Relaxed protobuf union validation strictness.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70546/#review214896 --- Ship it! Ship It! - Vinod Kone On April 25, 2019, 8:07 p.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70546/ > --- > > (Updated April 25, 2019, 8:07 p.m.) > > > Review request for mesos, Andrei Sekretenko, Benno Evers, and Greg Mann. > > > Bugs: MESOS-9740 > https://issues.apache.org/jira/browse/MESOS-9740 > > > Repository: mesos > > > Description > --- > > As part of MESOS-6874, the master validates protobuf unions passed as > part of an ExecutorInfo::ContainerInfo. This prevents a task from > specifying, for example, a ContainerInfo::MESOS, but filling > out the docker field (which is then ignored by the agent). > > This validation change is actually an API change, because previously > runnable ExecutorInfo's and TaskInfo's will now fail validation. > This has two visible effects on clusters: > * Agents running containers with invalid protobuf unions will not > be able to reregister with the master. > * Existing frameworks will not be able to re-launch the same tasks > that were working before a Mesos master upgrade. > > This changes the validation to print a warning instead. Where possible, > the warning will provide some information to indicate which task > or executor is sending the invalid protobuf. > > > Diffs > - > > src/common/validation.cpp 458f2258cc5fb76e65e2988dd3ab8bb827b0ac2d > src/master/validation.cpp d7f210fc1ed228113c7f97bce9a43916840b2252 > src/tests/master_validation_tests.cpp > c98f7517a1c29eea36f9a3c3da2cda1441967b77 > > > Diff: https://reviews.apache.org/r/70546/diff/1/ > > > Testing > --- > > > Thanks, > > Joseph Wu > >
Re: Review Request 70538: WIP: Fixed upgrade path for tasks with invalid protobuf unions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70538/#review214888 --- I would suggest we tweak the validation for 1.13.0 to log a warning instead of throwing an error and add a deprecation warning that "union" protobufs will be validated in a future release in the CHANGELOG. In the meantime, we can give times to frameworks to fix their bug, as well as figure what's the best way to handle existing tasks with invalid protos. - Vinod Kone On April 24, 2019, 3:28 a.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70538/ > --- > > (Updated April 24, 2019, 3:28 a.m.) > > > Review request for mesos, Andrei Sekretenko, Benno Evers, and Greg Mann. > > > Bugs: MESOS-9740 > https://issues.apache.org/jira/browse/MESOS-9740 > > > Repository: mesos > > > Description > --- > > As part of MESOS-6874, the master now validates protobuf unions > passed as part of an ExecutorInfo::ContainerInfo. This prevents a > task from specifying, for example, a ContainerInfo::MESOS, but filling > out the `docker` field (which is then ignored by the agent). > > However, if a task was already launched with an invalid protobuf > union, the same validation will happen when the agent tries to > reregister with the master. In this case, if the master is upgraded > to validate protobuf unions, the agent reregistration will be rejected. > > This adds a hack to wipe invalid fields from the agent's reregistration > message before sending it to the master. > > > Diffs > - > > src/slave/slave.cpp 95f05a18c7905d5032de1cd35726ac3a17f0b682 > > > Diff: https://reviews.apache.org/r/70538/diff/1/ > > > Testing > --- > > WIP!!! > > > Thanks, > > Joseph Wu > >
Re: Review Request 70449: Avoid publishing resources when an HTTP executor resubscribes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70449/#review214586 --- src/slave/slave.cpp Lines 5033 (patched) <https://reviews.apache.org/r/70449/#comment300762> s/updated/update/ - Vinod Kone On April 11, 2019, 1:30 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70449/ > --- > > (Updated April 11, 2019, 1:30 a.m.) > > > Review request for mesos, Benjamin Bannier, Greg Mann, and Vinod Kone. > > > Bugs: MESOS-9711 > https://issues.apache.org/jira/browse/MESOS-9711 > > > Repository: mesos > > > Description > --- > > After an agent failover, an HTTP executor may resubscribe before any > resource provider resubscribes. If that happens and the executor > has tasks consuming resources from an unsubscribed resource provider, > the agent will fail to publish the resources and kill the executor, > which is an undesired behavior. The patch fixes this issue. > > > Diffs > - > > src/slave/slave.cpp 794a9c986b266b4916f6fbada670142798245bd1 > src/tests/slave_tests.cpp b1c3a01031b917fb9773c8c890a8f88838870559 > > > Diff: https://reviews.apache.org/r/70449/diff/4/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 70450: Special cased HEARTBEAT call handling in agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70450/#review214561 --- Fix it, then Ship it! src/slave/http.cpp Lines 794 (patched) <https://reviews.apache.org/r/70450/#comment300734> How about: // We return early here before doing any validation because currently this proto contains // dummy values for framework and executor ids (which is safe). See TODO inside `heartbeat()` in `src/executor/executor.cpp`. - Vinod Kone On April 11, 2019, 12:02 a.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70450/ > --- > > (Updated April 11, 2019, 12:02 a.m.) > > > Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone. > > > Bugs: MESOS-9727 > https://issues.apache.org/jira/browse/MESOS-9727 > > > Repository: mesos > > > Description > --- > > This silences a '400 Bad Request' response by the agent whenever an > executor sends a HEARTBEAT call. These HEARTBEATs do not include a > valid value for required fields (FrameworkID and ExecutorID) because > they are not known by the library generating the HEARTBEATs. > > The error is harmless because HEARTBEAT calls do not have any effect > besides generating traffic. > > > Diffs > - > > src/executor/executor.cpp e9439da2afec3746403a1fc8fbc4420a67f8e509 > src/slave/http.cpp d66ae520f68b783caea8937a1a753608099d5926 > > > Diff: https://reviews.apache.org/r/70450/diff/2/ > > > Testing > --- > > make check > > > Thanks, > > Joseph Wu > >
Re: Review Request 70449: Avoid publishing resources when an HTTP executor resubscribes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70449/#review214560 --- Fix it, then Ship it! LGTM modulo test code that I'll let Greg review :) src/slave/slave.cpp Lines 5033 (patched) <https://reviews.apache.org/r/70449/#comment300733> s/We don't need to updated/It is safe to not update/ - Vinod Kone On April 10, 2019, 11:55 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70449/ > --- > > (Updated April 10, 2019, 11:55 p.m.) > > > Review request for mesos, Benjamin Bannier, Greg Mann, and Vinod Kone. > > > Bugs: MESOS-9711 > https://issues.apache.org/jira/browse/MESOS-9711 > > > Repository: mesos > > > Description > --- > > After an agent failover, an HTTP executor may resubscribe before any > resource provider resubscribes. If that happens and the executor > has tasks consuming resources from an unsubscribed resource provider, > the agent will fail to publish the resources and kill the executor, > which is an undesired behavior. The patch fixes this issue. > > > Diffs > - > > src/slave/slave.cpp 794a9c986b266b4916f6fbada670142798245bd1 > src/tests/slave_tests.cpp b1c3a01031b917fb9773c8c890a8f88838870559 > > > Diff: https://reviews.apache.org/r/70449/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 70450: Special cased HEARTBEAT call handling in agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70450/#review214558 --- src/slave/http.cpp Lines 793 (patched) <https://reviews.apache.org/r/70450/#comment300731> Can you add a TODO to actually send the correct framework and executor id in HEARTBEAT calls. It seems weird/wrong that we send dummy values. - Vinod Kone On April 10, 2019, 11:55 p.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70450/ > --- > > (Updated April 10, 2019, 11:55 p.m.) > > > Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > This silences a '400 Bad Request' response by the agent whenever an > executor sends a HEARTBEAT call. These HEARTBEATs do not include a > valid value for required fields (FrameworkID and ExecutorID) because > they are not known by the library generating the HEARTBEATs. > > The error is harmless because HEARTBEAT calls do not have any effect > besides generating traffic. > > > Diffs > - > > src/slave/http.cpp d66ae520f68b783caea8937a1a753608099d5926 > > > Diff: https://reviews.apache.org/r/70450/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Joseph Wu > >
Re: Review Request 70449: Avoid publishing resources during HTTP executor resubscriptions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70449/#review214556 --- I didn't look at the test. Also, typo in the description. s/resubscriptions/resubscribes/ src/slave/slave.cpp Lines 5031 (patched) <https://reviews.apache.org/r/70449/#comment300730> Can you expand a bit here on why not publishing the updated executor resources (e.g., some tasks might have terminated while the agent was down) is not an issue? I remember this being safe because of lifecycle of resources vs publish/unpublish. - Vinod Kone On April 10, 2019, 11:36 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70449/ > --- > > (Updated April 10, 2019, 11:36 p.m.) > > > Review request for mesos, Benjamin Bannier, Greg Mann, and Vinod Kone. > > > Bugs: MESOS-9711 > https://issues.apache.org/jira/browse/MESOS-9711 > > > Repository: mesos > > > Description > --- > > After an agent failover, an HTTP executor may resubscribe before > resource provider resubscriptions. If that happens and the executor > has tasks consuming resources from an unsubscribed resource provider, > the agent will fail to publish the resources and kill the executor, > which is an undesired behavior. The patch fixes this issue. > > > Diffs > - > > src/slave/slave.cpp 794a9c986b266b4916f6fbada670142798245bd1 > src/tests/slave_tests.cpp b1c3a01031b917fb9773c8c890a8f88838870559 > > > Diff: https://reviews.apache.org/r/70449/diff/2/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Review Request 70158: WIP.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70158/ --- Review request for mesos. Repository: mesos Description --- WIP. Diffs - NOTICE aa2fc8b2124bac35eb6e22c7188a8e38ef4d37a2 Diff: https://reviews.apache.org/r/70158/diff/1/ Testing --- Testing RB webhook trigger. Thanks, Vinod Kone
Re: Review Request 70103: Removed outdated scaling framework.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70103/#review213534 --- Ship it! Ship It! - Vinod Kone On March 3, 2019, 12:06 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70103/ > --- > > (Updated March 3, 2019, 12:06 p.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Repository: mesos > > > Description > --- > > This old code was never updated for recent changes, is not documented, > and currently does not work anymore. Removing it for now. Should we > decide to bring it back in the future we should make sure it is tested > automatically so it does not go out of sync again, and also that it is > documented and discoverable by users. > > > Diffs > - > > src/scaling/nested_exec 45ba1260e476ee20dfe013a0406f650639e9a800 > src/scaling/nested_exec.py 28ccc3f5af2d293be8d810f7082dd43388ab4435 > src/scaling/scaling_exec 3be8bc8b5bf7a1f4386db2dda82b8663d522d065 > src/scaling/scaling_exec.py e9f4a86b29485559323241587879fab946e69841 > src/scaling/scaling_sched 5c6c2476ed6a0b1275011c394f6834eef7b3a134 > src/scaling/scaling_sched.py 011e353b12465076d96378cccbc5a0ff88231844 > > > Diff: https://reviews.apache.org/r/70103/diff/1/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70129: Updated advanced contributing guide.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70129/#review213444 --- Ship it! Ship It! - Vinod Kone On March 5, 2019, 6:53 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70129/ > --- > > (Updated March 5, 2019, 6:53 p.m.) > > > Review request for mesos, Alexander Rukletsov and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Removed the instruction to create a reviewboard account > from the advanced contributing guide, since this instruction > became impossible to follow for contributors. > > > Diffs > - > > docs/advanced-contribution.md 28a314178f12cd5b8b7c5b43b1ecb564061c3fe1 > > > Diff: https://reviews.apache.org/r/70129/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70105: Increased timeout in `generate-endpoint-help.py`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70105/#review213430 --- Ship it! Ship It! - Vinod Kone On March 3, 2019, 6:03 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70105/ > --- > > (Updated March 3, 2019, 6:03 p.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Bugs: MESOS-9592 > https://issues.apache.org/jira/browse/MESOS-9592 > > > Repository: mesos > > > Description > --- > > In CI starting master or agent process often took longer than the > allowed 10s which then lead to query timeouts and failed builds. > > This patch increases the allowed time from 10s to 10min which should > allow the script to run to completion on even the most adverse machines. > > > Diffs > - > > support/generate-endpoint-help.py 91c13d797ea27737af42f5698d589b84664425c5 > > > Diff: https://reviews.apache.org/r/70105/diff/1/ > > > Testing > --- > > `./support/generate-endpoint-help.py -c _/bin` generated endpoint help in > `docs/endpoints`. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70060: Updated ReviewBot to verify reviews by checking for updates recursively.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70060/ --- (Updated March 1, 2019, 10:11 p.m.) Review request for mesos, Benjamin Bannier and Till Toenshoff. Changes --- Addressed bannier's comments. NNFR. Bugs: MESOS-4599 https://issues.apache.org/jira/browse/MESOS-4599 Repository: mesos Description --- If any of the dependent reviews has an updated diff or dependency, it now triggers the ReviewBot. Previously only updates to the tail review in the chain triggered the ReviewBot. Diffs (updated) - support/verify-reviews.py f03869abd74e920283bbb2acc9c71f5f0f30e554 Diff: https://reviews.apache.org/r/70060/diff/3/ Changes: https://reviews.apache.org/r/70060/diff/2-3/ Testing --- ? mesos git:(vinod/reviewbot_recursive_diff_time) ? ./support/verify-reviews.py -u mesos-review -p foo --skip-verify Checking if review: 65835 needs verification Skipping blocking review 65835 Checking if review: 65836 needs verification Latest review timestamp: 2018-02-28 14:43:21 Latest diff timestamp: 2018-02-28 13:55:43 Dependent review: https://reviews.apache.org/api/review-requests/65835/ Latest diff timestamp: 2018-02-28 13:55:32 Dependent review: https://reviews.apache.org/api/review-requests/65834/ Latest diff timestamp: 2018-02-28 13:55:22 Checking if review: 65820 needs verification Skipping blocking review 65820 Checking if review: 65821 needs verification Skipping blocking review 65821 Checking if review: 65847 needs verification Latest review timestamp: 2018-03-01 02:54:04 Latest diff timestamp: 2018-02-28 20:22:13 Dependent review: https://reviews.apache.org/api/review-requests/65845/ Latest diff timestamp: 2018-02-28 19:57:14 Dependent review: https://reviews.apache.org/api/review-requests/65844/ Latest diff timestamp: 2018-02-28 19:57:08 Dependent review: https://reviews.apache.org/api/review-requests/65821/ Latest diff timestamp: 2018-02-28 19:57:02 Dependent review: https://reviews.apache.org/api/review-requests/65820/ ... ... Thanks, Vinod Kone
Re: Review Request 70060: Updated ReviewBot to verify reviews by checking for updates recursively.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70060/ --- (Updated Feb. 28, 2019, 6:53 p.m.) Review request for mesos, Benjamin Bannier and Till Toenshoff. Changes --- Re-implemented the logic to make it easier to read. PTAL. It's easiest to read the diff between origin and 2. https://reviews.apache.org/r/70060/diff/2/ Summary (updated) - Updated ReviewBot to verify reviews by checking for updates recursively. Bugs: MESOS-4599 https://issues.apache.org/jira/browse/MESOS-4599 Repository: mesos Description (updated) --- If any of the dependent reviews has an updated diff or dependency, it now triggers the ReviewBot. Previously only updates to the tail review in the chain triggered the ReviewBot. Diffs (updated) - support/verify-reviews.py f03869abd74e920283bbb2acc9c71f5f0f30e554 Diff: https://reviews.apache.org/r/70060/diff/2/ Changes: https://reviews.apache.org/r/70060/diff/1-2/ Testing (updated) --- ? mesos git:(vinod/reviewbot_recursive_diff_time) ? ./support/verify-reviews.py -u mesos-review -p foo --skip-verify Checking if review: 65835 needs verification Skipping blocking review 65835 Checking if review: 65836 needs verification Latest review timestamp: 2018-02-28 14:43:21 Latest diff timestamp: 2018-02-28 13:55:43 Dependent review: https://reviews.apache.org/api/review-requests/65835/ Latest diff timestamp: 2018-02-28 13:55:32 Dependent review: https://reviews.apache.org/api/review-requests/65834/ Latest diff timestamp: 2018-02-28 13:55:22 Checking if review: 65820 needs verification Skipping blocking review 65820 Checking if review: 65821 needs verification Skipping blocking review 65821 Checking if review: 65847 needs verification Latest review timestamp: 2018-03-01 02:54:04 Latest diff timestamp: 2018-02-28 20:22:13 Dependent review: https://reviews.apache.org/api/review-requests/65845/ Latest diff timestamp: 2018-02-28 19:57:14 Dependent review: https://reviews.apache.org/api/review-requests/65844/ Latest diff timestamp: 2018-02-28 19:57:08 Dependent review: https://reviews.apache.org/api/review-requests/65821/ Latest diff timestamp: 2018-02-28 19:57:02 Dependent review: https://reviews.apache.org/api/review-requests/65820/ ... ... Thanks, Vinod Kone
Review Request 70060: Updated ReviewBot to catch diff updates recursively up the chain.
69481 : 2019-01-07 00:46:11 Latest diff timestamp of review 69676 : 2019-02-26 15:12:55 Latest diff timestamp of review 67997 : 2019-02-26 03:01:48 Latest diff timestamp of review 68162 : 2019-02-26 03:02:23 Latest diff timestamp of review 68163 : 2019-02-26 03:03:18 Latest diff timestamp of review 69547 : 2019-02-26 03:04:29 Latest diff timestamp of review 69579 : 2019-02-26 03:06:04 Latest diff timestamp of review 69613 : 2019-02-26 03:06:36 Latest diff timestamp of review 69614 : 2018-12-21 00:56:23 Latest diff timestamp: 2019-02-26 15:12:55 Checking if review: 67444 needs verification Latest diff timestamp of review 68118 : 2018-07-31 05:15:38 Latest diff timestamp of review 68119 : 2018-07-31 05:15:47 Latest diff timestamp of review 67444 : 2018-07-31 23:54:58 Latest diff timestamp: 2018-07-31 23:54:58 Latest dependency change timestamp: 2018-07-31 23:56:53 Checking if review: 69411 needs verification Latest diff timestamp of review 69411 : 2018-11-22 01:12:22 Latest diff timestamp: 2018-11-22 01:12:22 Latest dependency change timestamp: 2018-12-18 18:43:36 Checking if review: 69478 needs verification Skipping blocking review 69478 Checking if review: 69481 needs verification Skipping blocking review 69481 Checking if review: 70049 needs verification Latest diff timestamp of review 70047 : 2019-02-26 11:05:27 Latest diff timestamp of review 70049 : 2019-02-25 01:55:02 Latest diff timestamp: 2019-02-26 11:05:27 Checking if review: 70046 needs verification Skipping blocking review 70046 Checking if review: 70053 needs verification Latest diff timestamp of review 70046 : 2019-02-25 21:18:22 Latest diff timestamp of review 70053 : 2019-02-25 21:18:35 Latest diff timestamp: 2019-02-25 21:18:35 Checking if review: 69342 needs verification Skipping blocking review 69342 Checking if review: 69544 needs verification Skipping blocking review 69544 Checking if review: 67997 needs verification Skipping blocking review 67997 Checking if review: 68162 needs verification Skipping blocking review 68162 Checking if review: 68163 needs verification Skipping blocking review 68163 Checking if review: 69547 needs verification Skipping blocking review 69547 Checking if review: 69579 needs verification Skipping blocking review 69579 Checking if review: 69613 needs verification Skipping blocking review 69613 Checking if review: 69675 needs verification Skipping blocking review 69675 Checking if review: 69345 needs verification Skipping blocking review 69345 Checking if review: 69676 needs verification Skipping blocking review 69676 Checking if review: 70047 needs verification Skipping blocking review 70047 Checking if review: 66746 needs verification Latest diff timestamp of review 66746 : 2018-04-20 20:17:04 Latest diff timestamp: 2018-04-20 20:17:04 Checking if review: 70044 needs verification Latest diff timestamp of review 69977 : 2019-02-22 01:51:32 Latest diff timestamp of review 69978 : 2019-02-23 01:27:50 Latest diff timestamp of review 70044 : 2019-02-26 19:00:31 Latest diff timestamp: 2019-02-26 19:00:31 Checking if review: 69978 needs verification Skipping blocking review 69978 Checking if review: 67044 needs verification Latest diff timestamp of review 67044 : 2018-05-14 17:25:07 Latest diff timestamp: 2018-05-14 17:25:07 Latest dependency change timestamp: 2019-02-26 23:30:30 94 review requests need verification 66457 66554 66039 66344 66643 66778 63371 66864 54987 66628 66230 67173 67177 67215 64074 67301 67304 67344 66881 67601 67631 67788 67476 67425 66823 66822 67832 66048 67884 67994 67602 68033 68050 68123 68266 68383 68175 67960 68301 68543 68582 68609 65112 61818 68706 67022 69582 69694 69755 69735 69775 69588 69931 69955 69971 66644 70002 70023 67228 67921 67968 68187 65116 68736 68761 68807 68814 68508 68903 68919 68916 68090 69325 69403 69313 69423 67185 68654 69615 66649 68785 69551 69559 69701 67762 67825 69614 67444 69411 70049 70053 66746 70044 67044 1df088d3e18b39b328291b1adc13f5e7c45b9e84 Thanks, Vinod Kone
Review Request 70055: Output Review Request URL in Reviewbot output.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70055/ --- Review request for mesos, Benjamin Bannier and Till Toenshoff. Repository: mesos Description --- The URL makes it easy to get to the Review from log messages. Diffs - support/verify-reviews.py ab4888ae04ded555abb75c7cea468afa0ccb2ea8 Diff: https://reviews.apache.org/r/70055/diff/1/ Testing --- ./support/verify-reviews.py -u foo -p bar --skip-verify This just made sure I didn't make any syntax errors. Thanks, Vinod Kone
Re: Review Request 70045: Skipped tests when verifying `master` as part of review verification.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70045/#review213122 --- Fix it, then Ship it! support/jenkins/reviewbot.sh Line 31 (original), 31 (patched) <https://reviews.apache.org/r/70045/#comment298974> Add a comment here that we are not running tests for when building HEAD? support/jenkins/reviewbot.sh Lines 38 (patched) <https://reviews.apache.org/r/70045/#comment298973> I don't think this is needed because verify-reviews.py sets the ENVIRONMENT and other variables explicitly before verifying the review. The comment at #33 should probably moved to #27. - Vinod Kone On Feb. 23, 2019, 8:18 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70045/ > --- > > (Updated Feb. 23, 2019, 8:18 p.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Due to flakes test results reported by reviewbot need manual > verification so we can skip executing tests when validating the > `master` branch. This saves some execution time during validation. > > Incidentally with this patch a previous comment matches now matches > what is happening. > > > Diffs > - > > support/jenkins/reviewbot.sh 3cdd7f1c909c930a561ce2890212118579a47b73 > > > Diff: https://reviews.apache.org/r/70045/diff/1/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70037: Bumped SVN yum repo URL.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70037/#review213039 --- Ship it! Ship It! - Vinod Kone On Feb. 21, 2019, 7 p.m., Jie Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70037/ > --- > > (Updated Feb. 21, 2019, 7 p.m.) > > > Review request for mesos, James DeFelice and Vinod Kone. > > > Repository: mesos > > > Description > --- > > SVN 1.9 has been removed from the yum repo. This patch bumps it to SVN > 1.11. > > > Diffs > - > > support/packaging/centos/centos6.dockerfile > 457f48de8390988c88581683a67bf9e023e2117c > support/packaging/centos/centos7.dockerfile > b69cf3e6dfb098beffb5114f92a11105319ac303 > > > Diff: https://reviews.apache.org/r/70037/diff/1/ > > > Testing > --- > > tested locally by running support/packaging/centos/build-docker-rpmbuild.sh > > > Thanks, > > Jie Yu > >
Re: Review Request 70026: Increased the number of review requests queried by ReviewBot to 200.
review 69970 Checking if review: 69955 needs verification Latest diff timestamp: 2019-02-12 05:27:22 Checking if review: 69971 needs verification Latest diff timestamp: 2019-02-13 05:53:54 Checking if review: 69342 needs verification Skipping blocking review 69342 Checking if review: 69895 needs verification Skipping blocking review 69895 Checking if review: 69896 needs verification Skipping blocking review 69896 Checking if review: 69897 needs verification Skipping blocking review 69897 Checking if review: 69904 needs verification Skipping blocking review 69904 Checking if review: 69962 needs verification Skipping blocking review 69962 Checking if review: 69963 needs verification Skipping blocking review 69963 Checking if review: 69961 needs verification Skipping blocking review 69961 Checking if review: 69977 needs verification Skipping blocking review 69977 Checking if review: 66644 needs verification Latest diff timestamp: 2018-05-02 04:46:38 Checking if review: 69978 needs verification Latest diff timestamp: 2019-02-13 23:15:36 Checking if review: 69898 needs verification Skipping blocking review 69898 Checking if review: 70009 needs verification Skipping blocking review 70009 Checking if review: 69615 needs verification Latest diff timestamp: 2019-02-08 06:44:52 Checking if review: 70002 needs verification Latest diff timestamp: 2019-02-18 09:40:13 Checking if review: 69960 needs verification Skipping blocking review 69960 Checking if review: 69967 needs verification Skipping blocking review 69967 Checking if review: 69980 needs verification Skipping blocking review 69980 Checking if review: 70001 needs verification Latest diff timestamp: 2019-02-18 08:11:33 Checking if review: 70017 needs verification Skipping blocking review 70017 Checking if review: 70014 needs verification Latest diff timestamp: 2019-02-20 00:44:33 Checking if review: 70018 needs verification Latest diff timestamp: 2019-02-20 06:20:03 Checking if review: 69972 needs verification Skipping blocking review 69972 Checking if review: 69994 needs verification Skipping blocking review 69994 Checking if review: 70010 needs verification Skipping blocking review 70010 Checking if review: 70022 needs verification Latest diff timestamp: 2019-02-20 15:00:11 Checking if review: 70011 needs verification Latest diff timestamp: 2019-02-20 15:48:45 Checking if review: 70024 needs verification Skipping blocking review 70024 Checking if review: 70023 needs verification Latest diff timestamp: 2019-02-20 16:51:07 Checking if review: 70025 needs verification Latest diff timestamp: 2019-02-20 19:18:29 Checking if review: 70016 needs verification Skipping blocking review 70016 Checking if review: 67762 needs verification Latest diff timestamp: 2019-02-20 18:45:42 89 review requests need verification 66230 67044 67173 67177 67215 67228 64074 67301 67304 67344 66881 67601 67631 67788 67476 67425 67825 66823 66822 67832 66048 67884 67921 67968 67994 67602 68033 68050 67444 68187 68123 68266 68383 68175 67960 68301 68543 68582 68609 65112 65116 68736 68761 68807 68814 68508 68903 68919 68916 61818 68090 68706 67022 69287 69325 69403 69313 69423 67185 68654 66649 68785 69551 69559 69411 69614 69701 69582 69694 69755 69735 69775 69588 69869 69931 69955 69971 66644 69978 69615 70002 70001 70014 70018 70022 70011 70023 70025 67762 8effbf42ce2e7305973c7da5cf96760eed9da3d1 Thanks, Vinod Kone
Review Request 70026: Increased the number of review requests queried by ReviewBot to 200.
: 2019-02-13 05:53:54 Checking if review: 69342 needs verification Skipping blocking review 69342 Checking if review: 69895 needs verification Skipping blocking review 69895 Checking if review: 69896 needs verification Skipping blocking review 69896 Checking if review: 69897 needs verification Skipping blocking review 69897 Checking if review: 69904 needs verification Skipping blocking review 69904 Checking if review: 69962 needs verification Skipping blocking review 69962 Checking if review: 69963 needs verification Skipping blocking review 69963 Checking if review: 69961 needs verification Skipping blocking review 69961 Checking if review: 69977 needs verification Skipping blocking review 69977 Checking if review: 66644 needs verification Latest diff timestamp: 2018-05-02 04:46:38 Checking if review: 69978 needs verification Latest diff timestamp: 2019-02-13 23:15:36 Checking if review: 69898 needs verification Skipping blocking review 69898 Checking if review: 70009 needs verification Skipping blocking review 70009 Checking if review: 69615 needs verification Latest diff timestamp: 2019-02-08 06:44:52 Checking if review: 70002 needs verification Latest diff timestamp: 2019-02-18 09:40:13 Checking if review: 69960 needs verification Skipping blocking review 69960 Checking if review: 69967 needs verification Skipping blocking review 69967 Checking if review: 69980 needs verification Skipping blocking review 69980 Checking if review: 70001 needs verification Latest diff timestamp: 2019-02-18 08:11:33 Checking if review: 70017 needs verification Skipping blocking review 70017 Checking if review: 70014 needs verification Latest diff timestamp: 2019-02-20 00:44:33 Checking if review: 70018 needs verification Latest diff timestamp: 2019-02-20 06:20:03 Checking if review: 69972 needs verification Skipping blocking review 69972 Checking if review: 69994 needs verification Skipping blocking review 69994 Checking if review: 70010 needs verification Skipping blocking review 70010 Checking if review: 70022 needs verification Latest diff timestamp: 2019-02-20 15:00:11 Checking if review: 70011 needs verification Latest diff timestamp: 2019-02-20 15:48:45 Checking if review: 70024 needs verification Skipping blocking review 70024 Checking if review: 70023 needs verification Latest diff timestamp: 2019-02-20 16:51:07 Checking if review: 70025 needs verification Latest diff timestamp: 2019-02-20 19:18:29 Checking if review: 70016 needs verification Skipping blocking review 70016 Checking if review: 67762 needs verification Latest diff timestamp: 2019-02-20 18:45:42 89 review requests need verification 66230 67044 67173 67177 67215 67228 64074 67301 67304 67344 66881 67601 67631 67788 67476 67425 67825 66823 66822 67832 66048 67884 67921 67968 67994 67602 68033 68050 67444 68187 68123 68266 68383 68175 67960 68301 68543 68582 68609 65112 65116 68736 68761 68807 68814 68508 68903 68919 68916 61818 68090 68706 67022 69287 69325 69403 69313 69423 67185 68654 66649 68785 69551 69559 69411 69614 69701 69582 69694 69755 69735 69775 69588 69869 69931 69955 69971 66644 69978 69615 70002 70001 70014 70018 70022 70011 70023 70025 67762 8effbf42ce2e7305973c7da5cf96760eed9da3d1 Thanks, Vinod Kone
Re: Review Request 70012: Fixed type error in verify-reviews.py.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70012/#review212928 --- Ship it! Ship It! - Vinod Kone On Feb. 19, 2019, 8:45 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70012/ > --- > > (Updated Feb. 19, 2019, 8:45 p.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Bugs: MESOS-9582 > https://issues.apache.org/jira/browse/MESOS-9582 > > > Repository: mesos > > > Description > --- > > This patch fixes a type error introduced in `ccd1970d1be` where we > attempted to use a string API on undecoded UTF8 strings. With this > patch the string is properly decoded. > > > Diffs > - > > support/verify-reviews.py 2521ffb810eeca199cfde14f465d826ab242059a > > > Diff: https://reviews.apache.org/r/70012/diff/1/ > > > Testing > --- > > Testing with invalid credentials, expectation is that test execution fails > due to invalid credentials. > > Without this patch: > ``` > $ ./support/verify-reviews.py -u foo -p bar > Checking if review: 69955 needs verification > Latest diff timestamp: 2019-02-12 05:27:22 > Verifying review 69955 > Dependent review: https://reviews.apache.org/api/review-requests/69954/ > Dependent review: https://reviews.apache.org/api/review-requests/69905/ > Dependent review: https://reviews.apache.org/api/review-requests/69904/ > Dependent review: https://reviews.apache.org/api/review-requests/69898/ > Dependent review: https://reviews.apache.org/api/review-requests/69897/ > Dependent review: https://reviews.apache.org/api/review-requests/69896/ > Dependent review: https://reviews.apache.org/api/review-requests/69895/ > Dependent review: https://reviews.apache.org/api/review-requests/69894/ > Dependent review: https://reviews.apache.org/api/review-requests/69893/ > Dependent review: https://reviews.apache.org/api/review-requests/69892/ > Dependent review: https://reviews.apache.org/api/review-requests/69866/ > Dependent review: https://reviews.apache.org/api/review-requests/69858/ > Applying review 69895 > Applying review 69896 > Applying review 69897 > Applying review 69898 > Applying review 69904 > Applying review 69905 > Applying review 69954 > Traceback (most recent call last): > File "./support/verify-reviews.py", line 185, in apply_reviews > apply_review(review_request["id"]) > File "./support/verify-reviews.py", line 155, in apply_review > (sys.executable, review_id), SCRIPT_PATH) > File "./support/verify-reviews.py", line 117, in shell > command, stderr=subprocess.STDOUT, cwd=working_dir, shell=True) > File > "/usr/local/Cellar/python/3.7.2_1/Frameworks/Python.framework/Versions/3.7/lib/python3.7/subprocess.py", > line 395, in check_output > **kwargs).stdout > File > "/usr/local/Cellar/python/3.7.2_1/Frameworks/Python.framework/Versions/3.7/lib/python3.7/subprocess.py", > line 487, in run > output=stdout, stderr=stderr) > subprocess.CalledProcessError: Command 'cd .. && > /usr/local/opt/python/bin/python3.7 support/apply-reviews.py -n -r 69954' > returned non-zero exit status 1. > > During handling of the above exception, another exception occurred: > > Traceback (most recent call last): > File "./support/verify-reviews.py", line 378, in > main() > File "./support/verify-reviews.py", line 371, in main > verify_review(review_request) > File "./support/verify-reviews.py", line 222, in verify_review > apply_reviews(review_request, reviews) > File "./support/verify-reviews.py", line 177, in apply_reviews > apply_reviews(api(review_url)["review_request"], reviews) > File "./support/verify-reviews.py", line 187, in apply_reviews > if "patch does not apply" in err.output: > TypeError: a bytes-like object is required, not 'str' > ccd1970d1be47306d15cffc68edadfe3926d265a > ``` > > With this patch: > ``` > % ./support/verify-reviews.py -u foo -p bar > Checking if review: 69955 needs verification > Latest diff timestamp: 2019-02-12 05:27:22 > Verifying review 69955 > Dependent review: https://reviews.apache.org/api/review-requests/69954/ > Dependent review: https://reviews.apache.org/api/review-requests/69905/ > Dependent review: https://reviews.apache
Re: Review Request 70000: Fixed verify-reviews.py to not abort review verification prematurely.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7/#review212920 --- Ship it! Ship It! - Vinod Kone On Feb. 19, 2019, 5:23 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7/ > --- > > (Updated Feb. 19, 2019, 5:23 p.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Bugs: MESOS-9582 > https://issues.apache.org/jira/browse/MESOS-9582 > > > Repository: mesos > > > Description > --- > > Previously `support/verify-reviews.py` would abort prematurely > whenever a review a patch could not be applied. This was due to > the `shell` function used to call `support/apply-reviews.py` invoking > `exit(1)` on the first error which stopped iteration over all > outstanding reviews. > > In this patch that explicit call to `exit` is removed, and instead we > let a possible `subprocess.CalledProcessError` propagate up for it to > be handled at a higher level. Currently this will post a note on the > review in question to (1) notify the submitter, and (2) prevent the > review from being checked again. > > With the changes here the script will not stop verify reviews in such > cases. > > > Diffs > - > > support/verify-reviews.py a88a91f8ecb3794846d2d5eddee27ada770d55b9 > > > Diff: https://reviews.apache.org/r/7/diff/6/ > > > Testing > --- > > Ran the script (with a dummy user and password) on today's reviewboard state. > The script attempted to post a review on the last patch in the chain instead > of aborting (see the `TODO` in the code on why we weren't able to diagnose > the issue in the faulty patch with the current implementation). > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70000: Fixed verify-reviews.py to not abort review verification prematurely.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7/#review212917 --- support/verify-reviews.py Line 182 (original), 188 (patched) <https://reviews.apache.org/r/7/#comment298793> hmm. i don't think this is the right place to catch this exception. what if we don't get into this for loop at all if it's a single review chain? i think it should be in #197. - Vinod Kone On Feb. 19, 2019, 4:44 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7/ > --- > > (Updated Feb. 19, 2019, 4:44 p.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Bugs: MESOS-9582 > https://issues.apache.org/jira/browse/MESOS-9582 > > > Repository: mesos > > > Description > --- > > Previously `support/verify-reviews.py` would abort prematurely > whenever a review a patch could not be applied. This was due to > the `shell` function used to call `support/apply-reviews.py` invoking > `exit(1)` on the first error which stopped iteration over all > outstanding reviews. > > In this patch that explicit call to `exit` is removed, and instead we > let a possible `subprocess.CalledProcessError` propagate up for it to > be handled at a higher level. Currently this will post a note on the > review in question to (1) notify the submitter, and (2) prevent the > review from being checked again. > > With the changes here the script will not stop verify reviews in such > cases. > > > Diffs > - > > support/verify-reviews.py a88a91f8ecb3794846d2d5eddee27ada770d55b9 > > > Diff: https://reviews.apache.org/r/7/diff/3/ > > > Testing > --- > > Ran the script (with a dummy user and password) on today's reviewboard state. > The script attempted to post a review on the last patch in the chain instead > of aborting (see the `TODO` in the code on why we weren't able to diagnose > the issue in the faulty patch with the current implementation). > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 69909: Tested unreachable task behavior on agent GC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69909/ --- (Updated Feb. 15, 2019, 6:03 p.m.) Review request for mesos, Gastón Kleiman and Greg Mann. Changes --- Addressed Gaston's comments. NNFR. Bugs: MESOS-8887 https://issues.apache.org/jira/browse/MESOS-8887 Repository: mesos Description --- Updated `PartitionTest, RegistryGcByCount` test. This test fails without the previous patch. Diffs (updated) - src/tests/partition_tests.cpp 15af47b7d4328861a7523e669c2afc6a6574166b Diff: https://reviews.apache.org/r/69909/diff/2/ Changes: https://reviews.apache.org/r/69909/diff/1-2/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 69907: Fixed variable names in `Master::_doRegistryGC()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69907/ --- (Updated Feb. 15, 2019, 6:02 p.m.) Review request for mesos, Gastón Kleiman and Greg Mann. Changes --- Addressed gaston's comments. NNFR. Repository: mesos Description --- Substituted `slave` with `slaveId` to be consistent with the code base. No functional changes. Diffs (updated) - src/master/master.cpp 8e23785006b4f4962770d8bba163f977b8b33414 Diff: https://reviews.apache.org/r/69907/diff/2/ Changes: https://reviews.apache.org/r/69907/diff/1-2/ Testing --- make check Thanks, Vinod Kone
Review Request 69909: Tested unreachable task behavior on agent GC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69909/ --- Review request for mesos, Gastón Kleiman and Greg Mann. Bugs: MESOS-8887 https://issues.apache.org/jira/browse/MESOS-8887 Repository: mesos Description --- Updated `PartitionTest, RegistryGcByCount` test. This test fails without the previous patch. Diffs - src/tests/partition_tests.cpp 15af47b7d4328861a7523e669c2afc6a6574166b Diff: https://reviews.apache.org/r/69909/diff/1/ Testing --- make check Thanks, Vinod Kone
Review Request 69908: Removed unreachable tasks from `Master::Framework` on agent GC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69908/ --- Review request for mesos, Gastón Kleiman and Greg Mann. Bugs: MESOS-8887 https://issues.apache.org/jira/browse/MESOS-8887 Repository: mesos Description --- Unreachable tasks are stored in `Slaves` and `Framework` structs of the master, but they were only being removed from the former when an unreachable agent is GCed from the registry. This patch fixes it so that the latter is also cleaned up. Diffs - src/master/master.cpp 4c9ef2528d0ab74b565dcb03d05c5189d0aa0c0f Diff: https://reviews.apache.org/r/69908/diff/1/ Testing --- make check (Updated test in next patch) Thanks, Vinod Kone
Review Request 69907: Fixed variable names in `Master::_doRegistryGC()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69907/ --- Review request for mesos, Gastón Kleiman and Greg Mann. Repository: mesos Description --- Substituted `slave` with `slaveId` to be consistent with the code base. No functional changes. Diffs - src/master/master.cpp 4c9ef2528d0ab74b565dcb03d05c5189d0aa0c0f Diff: https://reviews.apache.org/r/69907/diff/1/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 69891: Sent operation updates to schedulers when agents are removed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69891/#review212551 --- src/master/master.cpp Lines 10950 (patched) <https://reviews.apache.org/r/69891/#comment298369> I don't know if sending UNREACHABLE for all the 3 cases when `_removeSlave` is called is the right way. I think we need to have a discussion around the agent state / task state / operation state for each of the cases. What happens if a framework reconciles the operation after this code gets executed. Will it always get OPERATION_UNREACHABLE? If not, then that would be cnofusing. Also, the operation status is not changed in-memory here. Is that intentional? - Vinod Kone On Feb. 5, 2019, 4:24 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69891/ > --- > > (Updated Feb. 5, 2019, 4:24 p.m.) > > > Review request for mesos, Gastón Kleiman and Joseph Wu. > > > Bugs: MESOS-9541 > https://issues.apache.org/jira/browse/MESOS-9541 > > > Repository: mesos > > > Description > --- > > This patch makes the master send operation updates when > an agent is removed and a framework has requested > feedback for a pending operation on that agent. > > > Diffs > - > > src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 > src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 > > > Diff: https://reviews.apache.org/r/69891/diff/1/ > > > Testing > --- > > `make check` > `bin/mesos-tests.sh --gtest_filter="*OperationUpdatesAfterAgentShutdown*" > --gtest_repeat=-1 --gtest_break_on_failure` > > > Thanks, > > Greg Mann > >
Re: Review Request 69839: Fixed scheduler library on multiple SUBSCRIBE requests per connection.
> On Jan. 25, 2019, 4:01 p.m., Vinod Kone wrote: > > src/scheduler/scheduler.cpp > > Line 234 (original), 234 (patched) > > <https://reviews.apache.org/r/69839/diff/1/?file=2122187#file2122187line234> > > > > s/Augmenting/Authenticating/ > > Alexander Rukletsov wrote: > Please no: authenticatee does not really authenticate, but adds > authentication-related headers to the request. Oh right. I remember that. How about "Adding authentication headers to ..." or something like that? "Augmenting ..." is very opaque and doesn't convey any useful information for someone looking at logs. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69839/#review212325 --- On Jan. 25, 2019, 3:41 p.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69839/ > --- > > (Updated Jan. 25, 2019, 3:41 p.m.) > > > Review request for mesos, Alexander Rukletsov and Vinod Kone. > > > Bugs: MESOS-9210 > https://issues.apache.org/jira/browse/MESOS-9210 > > > Repository: mesos > > > Description > --- > > The HTTP scheduler API dictates that on a single connection, the scheduler may > only send a single SUBSCRIBE request. Due to recent authentication related > changes, this contract got broken. This patch restores the contract and adds a > test validating that the library is enforcing it. > > > Diffs > - > > src/scheduler/scheduler.cpp cb24ba9c8e1d04b8c62bdf07b12758a61b3bf036 > src/tests/scheduler_tests.cpp b571bb1d20744b943580677a26db4c12c7c311d1 > > > Diff: https://reviews.apache.org/r/69839/diff/1/ > > > Testing > --- > > manual testing; > Running the included test without patching `scheduler.cpp` -> fails as the > master does in fact receive two SUBSCRIBE requests. > > `make check` > > > Thanks, > > Till Toenshoff > >
Re: Review Request 69839: Fixed scheduler library on multiple SUBSCRIBE requests per connection.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69839/#review212325 --- src/scheduler/scheduler.cpp Line 234 (original), 234 (patched) <https://reviews.apache.org/r/69839/#comment298083> s/Augmenting/Authenticating/ src/scheduler/scheduler.cpp Line 587 (original), 573 (patched) <https://reviews.apache.org/r/69839/#comment298087> s/to augment request// I didn't know "<< future" is thing! - Vinod Kone On Jan. 25, 2019, 3:41 p.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69839/ > --- > > (Updated Jan. 25, 2019, 3:41 p.m.) > > > Review request for mesos, Alexander Rukletsov and Vinod Kone. > > > Bugs: MESOS-9210 > https://issues.apache.org/jira/browse/MESOS-9210 > > > Repository: mesos > > > Description > --- > > The HTTP scheduler API dictates that on a single connection, the scheduler may > only send a single SUBSCRIBE request. Due to recent authentication related > changes, this contract got broken. This patch restores the contract and adds a > test validating that the library is enforcing it. > > > Diffs > - > > src/scheduler/scheduler.cpp cb24ba9c8e1d04b8c62bdf07b12758a61b3bf036 > src/tests/scheduler_tests.cpp b571bb1d20744b943580677a26db4c12c7c311d1 > > > Diff: https://reviews.apache.org/r/69839/diff/1/ > > > Testing > --- > > manual testing; > Running the included test without patching `scheduler.cpp` -> fails as the > master does in fact receive two SUBSCRIBE requests. > > `make check` > > > Thanks, > > Till Toenshoff > >
Re: Review Request 69817: Refactored 'support/verify-reviews.py' to be closer to commit 7412179.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69817/#review212250 --- Looks ok to me. I'll let Till ship this. Has this been tested? - Vinod Kone On Jan. 23, 2019, 11:47 a.m., Armand Grillet wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69817/ > --- > > (Updated Jan. 23, 2019, 11:47 a.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Repository: mesos > > > Description > --- > > The goal of this commit is to make 'verify-reviews.py' as close as > possible to its last known working state (commit > 74121798f24fca372180b8c4bc00b4df07d46240). > > The diff between 'verify-reviews.py' at this commit and 7412179 is > available here: https://www.diffchecker.com/cmfaM83O All the new > features added since 7412179 have been added again and the code > has been made Python 3 compatible again. > > The goal is to have a diff as minimal as possible with improvements > regarding logs so that we do not face CI issues anymore. Changes > have been made regarding how we run commands from the script so > that they are run from the repository instead of where the > script is being called. > > > Diffs > - > > support/verify-reviews.py 71326d34bb649e27a3a2901867d31a2a1fffd4e9 > > > Diff: https://reviews.apache.org/r/69817/diff/1/ > > > Testing > --- > > > Thanks, > > Armand Grillet > >
Re: Review Request 69775: Updated master fail() logging from FATAL to ERROR.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69775/#review212082 --- src/master/master.cpp Line 1627 (original), 1627 (patched) <https://reviews.apache.org/r/69775/#comment297688> This exited the process before, now we no longer do? Is that safe? - Vinod Kone On Jan. 16, 2019, 9:17 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69775/ > --- > > (Updated Jan. 16, 2019, 9:17 p.m.) > > > Review request for mesos, Andrei Budnik, Benjamin Mahler, Greg Mann, and Qian > Zhang. > > > Repository: mesos > > > Description > --- > > LOG(FATAL) would dump a stack trace which may confuse people with > a master crash case. We should just print out an error msg. > > > Diffs > - > > src/master/master.cpp 9624832d017dae717da803ca2ff2f8fc5135ea9d > > > Diff: https://reviews.apache.org/r/69775/diff/1/ > > > Testing > --- > > N/A > > > Thanks, > > Gilbert Song > >
Re: Review Request 69697: Reverted cleanup step of `verify-reviews.py`.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69697/#review212077
---
support/verify-reviews.py
Line 174 (original), 171 (patched)
<https://reviews.apache.org/r/69697/#comment297685>
Do you guys understand why this was failing in the Azure CI in the first
place? Originally, the HEAD variable was calculated outside `cleanup`. But
looks like at some point (during refactor for python3) it was pushed inside
`cleanup`. Do you know why?
Also `git checkout HEAD -- HEAD` is wrong. I don't think that works. Was
this tested?
- Vinod Kone
On Jan. 9, 2019, 9:30 a.m., Armand Grillet wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69697/
> ---
>
> (Updated Jan. 9, 2019, 9:30 a.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-9514
> https://issues.apache.org/jira/browse/MESOS-9514
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Reverts the improvements made on the support script `verify-reviews.py`
> in 3badf7179992e61f30f5a79da9d481dd451c7c2f. Sadly, these changes have
> broken our Azure based Mesos CI as seen in MESOS-9514.
>
> After comparing the code before and after the commit, we can see that
> the line failing in CI, `HEAD = shell("git rev-parse HEAD")`, is
> misplaced. It was previously located in the exit process in the
> `cleanup()` function and it is now back there, surrounded by a
> try/catch for error handling.
>
>
> Diffs
> -
>
> support/verify-reviews.py 72f98b234d9a2a84decb0569998d74e4c730122d
>
>
> Diff: https://reviews.apache.org/r/69697/diff/1/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 69705: Made agent not read the forked pid and libprocess pid after reboot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69705/#review211829 --- Can you write a unit test for this by spoofing the reboot? - Vinod Kone On Jan. 10, 2019, 2:52 p.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69705/ > --- > > (Updated Jan. 10, 2019, 2:52 p.m.) > > > Review request for mesos, Andrei Budnik and Gilbert Song. > > > Bugs: MESOS-9501 > https://issues.apache.org/jira/browse/MESOS-9501 > > > Repository: mesos > > > Description > --- > > After agent host is rebooted, the forked pid and libprocess pid in > agent's meta directory are obsolete, so we should not read them during > agent recovery, otherwise containerizer may wait for an irrelevant > process if the forked pid is reused by another process after reboot. > > > Diffs > - > > src/slave/state.hpp 4f3d4cefb3fdef29cce3a6abe4cf5db04d45301f > src/slave/state.cpp e7cf84993c74cf6da7fe22d5112e86e039780287 > > > Diff: https://reviews.apache.org/r/69705/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 69667: Sent SIGKILL to I/O switchboard server as a safeguard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69667/#review211733 --- Ship it! Ship It! - Vinod Kone On Jan. 5, 2019, 1:25 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69667/ > --- > > (Updated Jan. 5, 2019, 1:25 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-7042 > https://issues.apache.org/jira/browse/MESOS-7042 > > > Repository: mesos > > > Description > --- > > Sent SIGKILL to I/O switchboard server as a safeguard. > > > Diffs > - > > src/slave/containerizer/mesos/io/switchboard.cpp > c445a8f09d7671d5763281bac9881489b3cc9c39 > > > Diff: https://reviews.apache.org/r/69667/diff/2/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 69667: Sent SIGKILL to I/O switchboard server as a safeguard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69667/#review211676 --- src/slave/containerizer/mesos/io/switchboard.cpp Lines 817 (patched) <https://reviews.apache.org/r/69667/#comment297131> Add a comment for posterity. // If we are here, something really bad must have happened for I/O switchboard server // to not exit after SIGTERM has been sent. We have seen this happen due to FD leak (See: MESOS-9502). // We do a SIGKILL here as a safeguard, so that switchboard server forcefully exits and causes this // cleanup feature to be completed, thus unblocking the container's cleanup. - Vinod Kone On Jan. 4, 2019, 8:03 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69667/ > --- > > (Updated Jan. 4, 2019, 8:03 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-7042 > https://issues.apache.org/jira/browse/MESOS-7042 > > > Repository: mesos > > > Description > --- > > Sent SIGKILL to I/O switchboard server as a safeguard. > > > Diffs > - > > src/slave/containerizer/mesos/io/switchboard.cpp > c445a8f09d7671d5763281bac9881489b3cc9c39 > > > Diff: https://reviews.apache.org/r/69667/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 69667: Sent SIGKILL to I/O switchboard server as a safeguard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69667/#review211675 --- src/slave/containerizer/mesos/io/switchboard.cpp Lines 818 (patched) <https://reviews.apache.org/r/69667/#comment297129> I would put a much higher timeout, say 60s, to give enough time for sigterm handler (draining stdout/stderr) and also to make it more visible in the logs. also, log this at "ERROR" level since this is not expected to happen unless there is a bug? src/slave/containerizer/mesos/io/switchboard.cpp Lines 825 (patched) <https://reviews.apache.org/r/69667/#comment297130> Is there anway to test this? - Vinod Kone On Jan. 4, 2019, 8:03 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69667/ > --- > > (Updated Jan. 4, 2019, 8:03 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone. > > > Bugs: MESOS-7042 > https://issues.apache.org/jira/browse/MESOS-7042 > > > Repository: mesos > > > Description > --- > > Sent SIGKILL to I/O switchboard server as a safeguard. > > > Diffs > - > > src/slave/containerizer/mesos/io/switchboard.cpp > c445a8f09d7671d5763281bac9881489b3cc9c39 > > > Diff: https://reviews.apache.org/r/69667/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 69559: Simplified verify-reviews.py to be more similar to the Python 2 script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69559/#review211340 --- Ship it! Ship It! - Vinod Kone On Dec. 13, 2018, 3:59 p.m., Armand Grillet wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69559/ > --- > > (Updated Dec. 13, 2018, 3:59 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Dragos Schebesch, Till > Toenshoff, and Vinod Kone. > > > Bugs: MESOS-9459 > https://issues.apache.org/jira/browse/MESOS-9459 > > > Repository: mesos > > > Description > --- > > We have switched the Python 2 support scripts to Python 3 a few months > ago. During this transition, 'verify-reviews.py' has been modified, > mainly to work better on Windows: https://reviews.apache.org/r/67505/. > > The Python 3 script has then replaced the Python 2 one and we now face > an issue with the script, MESOS-9459. Due to all the changes made > between the Python 2 and Python 3 scripts, it is hard for us to > know when the script started failing and why. > > To solve this issue, this commit takes the state of 'verify-reviews.py' > as it was in 74121798f24fca372180b8c4bc00b4df07d46240, applies 2to3, > adds the '--out-file' option, then adds the content of: > * ac766c8bd0fba348399d8eac52f75f62e64db64e > * 43eba285cb5c8622a6be8a5bdad637ed53431e85 > * 5b348b6070f0d0403cb69b6a7fa638fc46b7ff49 > * 568fcdfd29788d9df89a51ffae7969c2bf0ea173 > > This gives a clean diff between 74121798f24fca372180b8c4bc00b4df07d46240 > and this commit, fixes the issue raised in MESOS-9459, and does not > remove existing features. However, this uses less our common library > developed in the chain that landed while migrating from Python 2 to 3. > > This should be followed by a refactoring of 'verify-review.py' and > 'common.py' to use our library more while not breaking the script > when using Reviewbot. > > > Diffs > - > > support/verify-reviews.py 552dc366a588e6c5ad0efdcdca03d66f1d7044c3 > > > Diff: https://reviews.apache.org/r/69559/diff/3/ > > > Testing > --- > > Checked https://www.diffchecker.com/R5uYlefc a diff of 'verify-reviews.py' > between 74121798f24fca372180b8c4bc00b4df07d46240 and this commit. > > > ``` > ? apache-mesos (MESOS-9459) ? BUILD_URL=yolo python3 > support/verify-reviews.py -u user -p password > git rev-parse HEAD > Checking if review: 69544 needs verification > Skipping blocking review 69544 > Checking if review: 69342 needs verification > Skipping blocking review 69342 > Checking if review: 69478 needs verification > Skipping blocking review 69478 > Checking if review: 69479 needs verification > Skipping blocking review 69479 > Checking if review: 67997 needs verification > Skipping blocking review 67997 > Checking if review: 68162 needs verification > Skipping blocking review 68162 > Checking if review: 68163 needs verification > Skipping blocking review 68163 > Checking if review: 69071 needs verification > Skipping blocking review 69071 > Checking if review: 68795 needs verification > Skipping blocking review 68795 > Checking if review: 69421 needs verification > Skipping blocking review 69421 > Checking if review: 69422 needs verification > Skipping blocking review 69422 > Checking if review: 69064 needs verification > Skipping blocking review 69064 > Checking if review: 69541 needs verification > Skipping blocking review 69541 > Checking if review: 68018 needs verification > Skipping blocking review 68018 > Checking if review: 68017 needs verification > Skipping blocking review 68017 > Checking if review: 68016 needs verification > Skipping blocking review 68016 > Checking if review: 69551 needs verification > Latest diff timestamp: 2018-12-13 14:20:34 > Verifying review 69551 > Applying review 69551 > /usr/local/opt/python/bin/python3.7 support/apply-reviews.py -n -r 69551 > Posting review: Bad patch! > > Reviews applied: [69551] > > Failed command: /usr/local/opt/python/bin/python3.7 support/apply-reviews.py > -n -r 69551 > > Error: > 2018-12-14 13:30:42 URL:https://reviews.apache.org/r/69551/diff/raw/ > [52606/52606] -> "69551.patch" [1] > Done processing src/tests/oversubscription_tests.cpp > Done processing src/tests/gc_tests.cpp > Done processing src/tests/containerizer/xfs_quota_tests.cpp > Done processing src/tests/log_tests.cpp > Done processing src/te
Re: Review Request 69559: Simplified verify-reviews.py to be more similar to the Python 2 script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69559/#review211306 --- git reset --hard b'69296553d96ca78b3145b9dd400a3c2332b441c7\n' Failed command: git reset --hard b'69296553d96ca78b3145b9dd400a3c2332b441c7\n' Error: b'fatal: Cannot do hard reset with paths.\n' This looks bad? Looks like we need to strip the trailing "\n"? - Vinod Kone On Dec. 13, 2018, 3:59 p.m., Armand Grillet wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69559/ > --- > > (Updated Dec. 13, 2018, 3:59 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Dragos Schebesch, Till > Toenshoff, and Vinod Kone. > > > Bugs: MESOS-9459 > https://issues.apache.org/jira/browse/MESOS-9459 > > > Repository: mesos > > > Description > --- > > We have switched the Python 2 support scripts to Python 3 a few months > ago. During this transition, 'verify-reviews.py' has been modified, > mainly to work better on Windows: https://reviews.apache.org/r/67505/. > > The Python 3 script has then replaced the Python 2 one and we now face > an issue with the script, MESOS-9459. Due to all the changes made > between the Python 2 and Python 3 scripts, it is hard for us to > know when the script started failing and why. > > To solve this issue, this commit takes the state of 'verify-reviews.py' > as it was in 74121798f24fca372180b8c4bc00b4df07d46240, applies 2to3, > adds the '--out-file' option, then adds the content of: > * ac766c8bd0fba348399d8eac52f75f62e64db64e > * 43eba285cb5c8622a6be8a5bdad637ed53431e85 > * 5b348b6070f0d0403cb69b6a7fa638fc46b7ff49 > * 568fcdfd29788d9df89a51ffae7969c2bf0ea173 > > This gives a clean diff between 74121798f24fca372180b8c4bc00b4df07d46240 > and this commit, fixes the issue raised in MESOS-9459, and does not > remove existing features. However, this uses less our common library > developed in the chain that landed while migrating from Python 2 to 3. > > This should be followed by a refactoring of 'verify-review.py' and > 'common.py' to use our library more while not breaking the script > when using Reviewbot. > > > Diffs > - > > support/verify-reviews.py 552dc366a588e6c5ad0efdcdca03d66f1d7044c3 > > > Diff: https://reviews.apache.org/r/69559/diff/2/ > > > Testing > --- > > Checked https://www.diffchecker.com/R5uYlefc a diff of 'verify-reviews.py' > between 74121798f24fca372180b8c4bc00b4df07d46240 and this commit. > > > ``` > ? apache-mesos (MESOS-9459) ? BUILD_URL=yolo python3 > support/verify-reviews.py -u user -p password > git rev-parse HEAD > Checking if review: 69342 needs verification > Skipping blocking review 69342 > Checking if review: 69478 needs verification > Skipping blocking review 69478 > Checking if review: 69479 needs verification > Skipping blocking review 69479 > Checking if review: 67997 needs verification > Skipping blocking review 67997 > Checking if review: 68162 needs verification > Skipping blocking review 68162 > Checking if review: 68163 needs verification > Skipping blocking review 68163 > Checking if review: 69547 needs verification > Latest diff timestamp: 2018-12-11 12:15:07 > Verifying review 69547 > Posting review: Bad review! > > Reviews applied: [] > > Error: > No reviewers specified. Please find a reviewer by asking on JIRA or the > mailing list. > git clean -fd > git reset --hard b'69296553d96ca78b3145b9dd400a3c2332b441c7\n' > Failed command: git reset --hard b'69296553d96ca78b3145b9dd400a3c2332b441c7\n' > > Error: b'fatal: Cannot do hard reset with paths.\n' > Checking if review: 69557 needs verification > Latest diff timestamp: 2018-12-12 11:16:24 > Verifying review 69557 > Dependent review: https://reviews.apache.org/api/review-requests/69398/ > Dependent review: https://reviews.apache.org/api/review-requests/69397/ > Applying review 69397 > /usr/local/opt/python/bin/python3.7 support/apply-reviews.py -n -r 69397 > ``` > > > Thanks, > > Armand Grillet > >
Re: Review Request 69559: Simplified verify-reviews.py to be more similar to the Python 2 script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69559/#review211263 --- LGTM. Have you tested this script by running locally? support/verify-reviews.py Lines 312 (patched) <https://reviews.apache.org/r/69559/#comment296201> this function name is weird. i would've called it `write_reviewids` since this function doesn't know/care anything about verification. but not sure if you want to do it in a different review for cleanliness. - Vinod Kone On Dec. 12, 2018, 2:19 p.m., Armand Grillet wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69559/ > --- > > (Updated Dec. 12, 2018, 2:19 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Dragos Schebesch, and Vinod > Kone. > > > Bugs: MESOS-9459 > https://issues.apache.org/jira/browse/MESOS-9459 > > > Repository: mesos > > > Description > --- > > We have switched the Python 2 support scripts to Python 3 a few months > ago. During this transition, 'verify-reviews.py' has been modified, > mainly to work better on Windows: https://reviews.apache.org/r/67505/. > > The Python 3 script has then replaced the Python 2 one and we now face > an issue with the script, MESOS-9459. Due to all the changes made > between the Python 2 and Python 3 scripts, it is hard for us to > know when the script started failing and why. > > To solve this issue, this commit takes the state of 'verify-reviews.py' > as it was in 74121798f24fca372180b8c4bc00b4df07d46240, applies 2to3, > adds the '--out-file' option, then adds the content of: > * ac766c8bd0fba348399d8eac52f75f62e64db64e > * 43eba285cb5c8622a6be8a5bdad637ed53431e85 > * 5b348b6070f0d0403cb69b6a7fa638fc46b7ff49 > * 568fcdfd29788d9df89a51ffae7969c2bf0ea173 > > This gives a clean diff between 74121798f24fca372180b8c4bc00b4df07d46240 > and this commit, fixes the issue raised in MESOS-9459, and does not > remove existing features. However, this uses less our common library > developed in the chain that landed while migrating from Python 2 to 3. > > This should be followed by a refactoring of 'verify-review.py' and > 'common.py' to use our library more while not breaking the script > when using Reviewbot. > > > Diffs > - > > support/verify-reviews.py 552dc366a588e6c5ad0efdcdca03d66f1d7044c3 > > > Diff: https://reviews.apache.org/r/69559/diff/1/ > > > Testing > --- > > Checked https://www.diffchecker.com/R5uYlefc a diff of 'verify-reviews.py' > between 74121798f24fca372180b8c4bc00b4df07d46240 and this commit. > > > Thanks, > > Armand Grillet > >
Review Request 69513: Manually copy test reports to host fs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69513/ --- Review request for mesos and James Peach. Repository: mesos Description --- Manually copy test reports to host fs. Diffs - support/mesos-build/entrypoint.sh ec98cc8b1fdcd0a32ed32cfeb69dfb976f82b81d Diff: https://reviews.apache.org/r/69513/diff/1/ Testing --- Have been testing this over week in a custom Jenkins job. Thanks, Vinod Kone
Re: Review Request 61128: Improved log messages in master when adding/removing tasks/executors.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61128/ --- (Updated Nov. 15, 2018, 9:40 p.m.) Review request for mesos, Benno Evers and Qian Zhang. Repository: mesos Description --- Made the log messages and the calling sites consistent and also added one for adding an executor. Diffs (updated) - src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40 Diff: https://reviews.apache.org/r/61128/diff/3/ Changes: https://reviews.apache.org/r/61128/diff/2-3/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 69293: Disabled parallel test execution for reviewbot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69293/#review210401 --- Ship it! Ship It! - Vinod Kone On Nov. 7, 2018, 10:57 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69293/ > --- > > (Updated Nov. 7, 2018, 10:57 p.m.) > > > Review request for mesos, Benjamin Mahler and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Currently no dedicated handling of XML reporting from multiple gtest > shards is implemented in our gtest runner. > > This patch disables the now default-enabled parallel test runner for > any build & tests cycles under the ASF CI reviewbot job. > > > Diffs > - > > support/jenkins/reviewbot.sh 0dd81fd1c067dfe84c4e638ebcee87bc6d4d73a7 > > > Diff: https://reviews.apache.org/r/69293/diff/1/ > > > Testing > --- > > NOTE: The flags for `Mesos-Buildbot` are configured directly in Jenkins and > already adjusted. > > > Thanks, > > Benjamin Bannier > >
Review Request 69291: Simplified writing out test report xml files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69291/ --- Review request for mesos and James Peach. Repository: mesos Description --- Simplified writing out test report xml files. Diffs - support/mesos-build/entrypoint.sh 012f003b3cf22b49a442df293f1b6224c074e383 Diff: https://reviews.apache.org/r/69291/diff/1/ Testing --- Thanks, Vinod Kone
Re: Review Request 69291: Simplified writing out test report xml files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69291/ --- (Updated Nov. 7, 2018, 10:36 p.m.) Review request for mesos and James Peach. Changes --- trailing slash. Repository: mesos Description --- Simplified writing out test report xml files. Diffs (updated) - support/mesos-build/entrypoint.sh 012f003b3cf22b49a442df293f1b6224c074e383 Diff: https://reviews.apache.org/r/69291/diff/2/ Changes: https://reviews.apache.org/r/69291/diff/1-2/ Testing --- Thanks, Vinod Kone
Review Request 69279: Updated Build bot to write out test report xml files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69279/ --- Review request for mesos and James Peach. Repository: mesos Description --- This can be used by Jenkins to display test trends. Diffs - support/mesos-build.sh d146cc104ea330bfdcbfed497267e6a1465db0bc support/mesos-build/entrypoint.sh ec98cc8b1fdcd0a32ed32cfeb69dfb976f82b81d Diff: https://reviews.apache.org/r/69279/diff/1/ Testing --- Tested manually with a custom jenkins job. Thanks, Vinod Kone
Re: Review Request 69193: Added filters info to the `DECLINE` call log message.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69193/#review210099 --- Ship it! Ship It! - Vinod Kone On Oct. 26, 2018, 5:42 p.m., Meng Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69193/ > --- > > (Updated Oct. 26, 2018, 5:42 p.m.) > > > Review request for mesos, Benjamin Mahler and Gastón Kleiman. > > > Repository: mesos > > > Description > --- > > This helps to investigate allocator allocation decisions > without verbose logging. > > > Diffs > - > > src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a > > > Diff: https://reviews.apache.org/r/69193/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Meng Zhu > >
Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69123/#review209921 --- src/linux/cgroups.cpp Line 1124 (original), 1137 (patched) <https://reviews.apache.org/r/69123/#comment294558> Didn't quite follow why you had make this an Option? - Vinod Kone On Oct. 23, 2018, 3:08 a.m., Benjamin Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69123/ > --- > > (Updated Oct. 23, 2018, 3:08 a.m.) > > > Review request for mesos and Qian Zhang. > > > Repository: mesos > > > Description > --- > > The cgroups event notifier was closing the eventfd while an > `io::read()` operation may be in progress. This can lead to > bugs where the fd gets re-used and read from a stale io::read. > > > Diffs > - > > src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 > > > Diff: https://reviews.apache.org/r/69123/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Benjamin Mahler > >
Re: Review Request 68951: Updated verify-reviews.py to use current interpreter in subprocesses.
> On Oct. 8, 2018, 6:12 p.m., Till Toenshoff wrote:
> > support/verify-reviews.py
> > Line 97 (original), 97 (patched)
> > <https://reviews.apache.org/r/68951/diff/1/?file=2095334#file2095334line97>
> >
> > IIUC, then `sys.executable` may be `None`. Shall we guard against that?
>
> Armand Grillet wrote:
> As described in
> https://docs.python.org/3/library/sys.html#sys.executable: "If Python is
> unable to retrieve the real path to its executable, sys.executable will be an
> empty string or None.".
> In the first case, "", the shebang in the first line of
> `support/apply-reviews.py`
> (https://github.com/apache/mesos/blob/8375e426d1c07f625ee18cd439e0dd4f1dc804c5/support/apply-reviews.py#L1)
> will make us use Python 3 AFAIK.
>
> The reason why I set the current interpreter instead of nothing is that a
> user might specify an interpreter when running `support/apply-reviews.py`.
> This is not the first time we do this in our support scripts:
> https://github.com/apache/mesos/blob/8375e426d1c07f625ee18cd439e0dd4f1dc804c5/support/mesos-style.py#L291
i'm dropping this and committing. spoke with @till offline.
- Vinod
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68951/#review209317
---
On Oct. 8, 2018, 6:06 p.m., Armand Grillet wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68951/
> ---
>
> (Updated Oct. 8, 2018, 6:06 p.m.)
>
>
> Review request for mesos, Till Toenshoff and Vinod Kone.
>
>
> Bugs: MESOS-9253
> https://issues.apache.org/jira/browse/MESOS-9253
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This changes the command used in `support/verify-reviews.py` when
> running `support/apply-reviews.py` as a subprocess. It was previously
> `"python"`, which is generally Python 2, and is now `sys.executable`.
>
> That way, if verify-reviews.py is run with Python 3 (as it should),
> apply-reviews.py will be run with the same Python 3 interpreter. This
> should fix the `ImportError` issues we have recently seen in our CI.
>
>
> Diffs
> -
>
> support/verify-reviews.py 56321ae65b38a1c62f5589b6a8aaa3993fa3dd5f
>
>
> Diff: https://reviews.apache.org/r/68951/diff/1/
>
>
> Testing
> ---
>
> I have created a simple `test.py` file to check that the interpreter was
> correctly found:
>
> ```
> import sys
>
>
> def apply_review(review_id):
> """Apply a review using the script apply-reviews.py."""
> print("Applying review %s" % review_id)
> print("%s support/apply-reviews.py -n -r %s" % (sys.executable,
> review_id))
>
> apply_review(1337)
> apply_review("1337")
> ```
>
> In both cases, `python3 test.py` prints `/usr/local/bin/python3
> support/apply-reviews.py -n -r 1337` which is what I expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 68951: Updated verify-reviews.py to use current interpreter in subprocesses.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68951/#review209324
---
Ship it!
Ship It!
- Vinod Kone
On Oct. 8, 2018, 6:06 p.m., Armand Grillet wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68951/
> ---
>
> (Updated Oct. 8, 2018, 6:06 p.m.)
>
>
> Review request for mesos, Till Toenshoff and Vinod Kone.
>
>
> Bugs: MESOS-9253
> https://issues.apache.org/jira/browse/MESOS-9253
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This changes the command used in `support/verify-reviews.py` when
> running `support/apply-reviews.py` as a subprocess. It was previously
> `"python"`, which is generally Python 2, and is now `sys.executable`.
>
> That way, if verify-reviews.py is run with Python 3 (as it should),
> apply-reviews.py will be run with the same Python 3 interpreter. This
> should fix the `ImportError` issues we have recently seen in our CI.
>
>
> Diffs
> -
>
> support/verify-reviews.py 56321ae65b38a1c62f5589b6a8aaa3993fa3dd5f
>
>
> Diff: https://reviews.apache.org/r/68951/diff/1/
>
>
> Testing
> ---
>
> I have created a simple `test.py` file to check that the interpreter was
> correctly found:
>
> ```
> import sys
>
>
> def apply_review(review_id):
> """Apply a review using the script apply-reviews.py."""
> print("Applying review %s" % review_id)
> print("%s support/apply-reviews.py -n -r %s" % (sys.executable,
> review_id))
>
> apply_review(1337)
> apply_review("1337")
> ```
>
> In both cases, `python3 test.py` prints `/usr/local/bin/python3
> support/apply-reviews.py -n -r 1337` which is what I expected.
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 68912: Added a log line to `MesosContainerizer::kill()`.
> On Oct. 3, 2018, 4:05 p.m., Mesos Reviewbot Windows wrote: > > FAIL: Some of the unit tests failed. Please check the relevant logs. > > > > Reviews applied: `['68912']` > > > > Failed command: `Start-MesosCITesting` > > > > All the build artifacts available at: > > http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2413/mesos-review-68912 > > > > Relevant logs: > > > > - > > [mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2413/mesos-review-68912/logs/mesos-tests.log): > > > > ``` > > W1003 16:05:33.435480 12912 slave.cpp:3917] Ignoring shutdown framework > > d7d1a6b8-7f3b-439c-a27a-847a8acd066c- because it is terminating > > I1003 16:05:33.436484 15652 master.cpp:1251] Agent > > d7d1a6b8-7f3b-439c-a27a-847a8acd066c-S0 at slave(461)@192.10.1.5:54865 > > (windows-02.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net) > > disconnected > > I1003 16:05:33.437479 15652 master.cpp:3267] Disconnecting agent > > d7d1a6b8-7f3b-439c-a27a-847a8acd066c-S0 at slave(461)@192.10.1.5:54865 > > (windows-02.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net) > > I1003 16:05:33.437479 15652 master.cpp:3286] Deactivating agent > > d7d1a6b8-7f3b-439c-a27a-847a8acd066c-S0 at slave(461)@192.10.1.5:54865 > > (windows-02.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net) > > I1003 16:05:33.437479 7924 hierarchical.cpp:359] Removed framework > > d7d1a6b8-7f3b-439c-a27a-847a8acd066c- > > I1003 16:05:33.437479 7924 hierarchical.cpp:803] Agent > > d7d1a6b8-7f3b-439c-a27a-847a8acd066c-S0 deactivated > > I1003 16:05:33.439498 8116 containerizer.cpp:2455] Destroying container > > 0da637c0-98a8-4ad4-ac65-6cf607cbba39 in RUNNING state > > I1003 16:05:33.439498 8116 containerizer.cpp:3122] Transitioning the state > > of container 0da637c0-98a8-4ad4-ac65-6cf607cbba39 from RUNNING to DESTROYING > > I1003 16:05:33.439498 8116 launcher.cpp:166] Asked to destroy containe[ > >OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (584 ms) > > [--] 1 test from IsolationFlag/MemoryIsolatorTest (603 ms total) > > > > [--] Global test environment tear-down > > d:\dcos\mesos\mesos\src\tests\environment.cpp(1126): error: Failed > > Tests completed with child processes remaining: > > -+- 1244 0024FFD8E6DC > > --- 15936 0024FFD8E6DC > > [==] 1051 tests from 103 test cases ran. (489504 ms total) > > [ PASSED ] 1051 tests. > > [ FAILED ] 0 tests, listed below: > > > > 0 FAILED TESTS > > > > YOU HAVE 232 DISABLED TESTS > > > > r 0da637c0-98a8-4ad4-ac65-6cf607cbba39 > > I1003 16:05:33.471307 7924 containerizer.cpp:2961] Container > > 0da637c0-98a8-4ad4-ac65-6cf607cbba39 has exited > > I1003 16:05:33.500296 14928 master.cpp:1093] Master terminating > > I1003 16:05:33.502277 15652 hierarchical.cpp:645] Removed agent > > d7d1a6b8-7f3b-439c-a27a-847a8acd066c-S0 > > I1003 16:05:33.824251 17340 process.cpp:926] Stopped the socket accept loop > > ``` @andy Is this a known issue with windows reviewbot? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68912/#review209187 --- On Oct. 3, 2018, 3:05 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68912/ > --- > > (Updated Oct. 3, 2018, 3:05 p.m.) > > > Review request for mesos and Gilbert Song. > > > Repository: mesos > > > Description > --- > > Added a log line to `MesosContainerizer::kill()`. > > > Diffs > - > > src/slave/containerizer/mesos/containerizer.cpp > 82d64e9928abd5e22493fa697d7b0910211533f3 > > > Diff: https://reviews.apache.org/r/68912/diff/1/ > > > Testing > --- > > make -j3 check > > Ran it through internal CI. > > > Thanks, > > Vinod Kone > >
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/#review209186 --- Ship it! Ship It! - Vinod Kone On Sept. 27, 2018, 5:41 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68866/ > --- > > (Updated Sept. 27, 2018, 5:41 p.m.) > > > Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. > > > Bugs: MESOS-9274 > https://issues.apache.org/jira/browse/MESOS-9274 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp > 2a5fbd79ac7bad933067cd96e38186849af8edc4 > > > Diff: https://reviews.apache.org/r/68866/diff/1/ > > > Testing > --- > > `make check`on various Linux distro > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
> On Sept. 27, 2018, 6:23 p.m., Vinod Kone wrote: > > src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp > > Lines 294 (patched) > > <https://reviews.apache.org/r/68866/diff/1/?file=2092453#file2092453line294> > > > > Looks like you are doing `call` here instead of `send` so that you have > > a future to wait on? Skipping `send` which does validation and > > authentication seems wrong. Let's not do that. > > > > I would recommend putting a sleep in the client code instead of here > > for now. > > Alexander Rukletsov wrote: > But `call` does > [validation](https://github.com/apache/mesos/blob/6e21e94ddca5b776d44636fe3eba8500bf88dc25/src/scheduler/scheduler.cpp#L269-L270) > and > [authentication](https://github.com/apache/mesos/blob/6e21e94ddca5b776d44636fe3eba8500bf88dc25/src/scheduler/scheduler.cpp#L301) > too. Not sure I follow, Vinod. > > Greg Mann wrote: > Yea I think we have the necessary validation/authentication in the > `call()` path as well. This solution is nice because it will wait for as > little time as possible, up to 5 seconds, rather than always waiting for 5 > seconds. > > In fact, since this code will return early if a response is actually > received, I would feel comfortable with a slightly longer await - maybe 10 or > 20 seconds? I didn't realize `call` was a newish interface method added to the library. At a quick glance I thought it was the continuation of `send` and an internal method. My bad. So I'm ok with doing a `call` here. Suggestion: The comment above doesn't have enough context for a future to understand why the library could be destructured. I would recommend expanding the comment a bit more explaining. For example "The JVM could potentially call JNI `finalize` if the Java scheduler nullified its reference to `V1Mesos` library immediately after sending TEARDOWN." Also, maybe link to MESOS-9274 for posterity. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/#review209074 --- On Sept. 27, 2018, 5:41 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68866/ > --- > > (Updated Sept. 27, 2018, 5:41 p.m.) > > > Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. > > > Bugs: MESOS-9274 > https://issues.apache.org/jira/browse/MESOS-9274 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp > 2a5fbd79ac7bad933067cd96e38186849af8edc4 > > > Diff: https://reviews.apache.org/r/68866/diff/1/ > > > Testing > --- > > `make check`on various Linux distro > > > Thanks, > > Alexander Rukletsov > >
Review Request 68912: Added a log line to `MesosContainerizer::kill()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68912/ --- Review request for mesos and Gilbert Song. Repository: mesos Description --- Added a log line to `MesosContainerizer::kill()`. Diffs - src/slave/containerizer/mesos/containerizer.cpp 82d64e9928abd5e22493fa697d7b0910211533f3 Diff: https://reviews.apache.org/r/68912/diff/1/ Testing --- make -j3 check Ran it through internal CI. Thanks, Vinod Kone
Re: Review Request 68899: Updated the MesosCon 2018 location.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68899/#review209156 --- Ship it! Ship It! - Vinod Kone On Oct. 2, 2018, 6:59 p.m., Gastón Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68899/ > --- > > (Updated Oct. 2, 2018, 6:59 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Updated the MesosCon 2018 location. > > > Diffs > - > > site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md > 09cf1a85f5d497c421d96c97bb855b9a486ccc0d > > > Diff: https://reviews.apache.org/r/68899/diff/1/ > > > Testing > --- > > Built the website using `site/mesos-website-dev.sh`, see the attached > screenshot. > > > File Attachments > > > Blog post screenshot > > https://reviews.apache.org/media/uploaded/files/2018/10/02/ddbc84c9-ff2f-44f0-b68e-c6bc9070b11a__screenshot.png > > > Thanks, > > Gastón Kleiman > >
Re: Review Request 68865: Put `TerminateEvent` at the end of the queue in the Mesos library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68865/#review209075 --- src/scheduler/scheduler.cpp Line 1041 (original), 1041 (patched) <https://reviews.apache.org/r/68865/#comment293336> Can you add a comment on why you are doing this for posterity? - Vinod Kone On Sept. 27, 2018, 5:41 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68865/ > --- > > (Updated Sept. 27, 2018, 5:41 p.m.) > > > Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. > > > Bugs: MESOS-9274 > https://issues.apache.org/jira/browse/MESOS-9274 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/scheduler/scheduler.cpp 471152945d6af660c8983324b38702d872657f89 > > > Diff: https://reviews.apache.org/r/68865/diff/1/ > > > Testing > --- > > See https://reviews.apache.org/r/68866/ > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/#review209074 --- src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp Lines 294 (patched) <https://reviews.apache.org/r/68866/#comment293335> Looks like you are doing `call` here instead of `send` so that you have a future to wait on? Skipping `send` which does validation and authentication seems wrong. Let's not do that. I would recommend putting a sleep in the client code instead of here for now. - Vinod Kone On Sept. 27, 2018, 5:41 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68866/ > --- > > (Updated Sept. 27, 2018, 5:41 p.m.) > > > Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. > > > Bugs: MESOS-9274 > https://issues.apache.org/jira/browse/MESOS-9274 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp > 2a5fbd79ac7bad933067cd96e38186849af8edc4 > > > Diff: https://reviews.apache.org/r/68866/diff/1/ > > > Testing > --- > > `make check`on various Linux distro > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 68826: Fixed bug in `verify-reviews` due to mismatched types.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68826/#review208959 --- Ship it! LGTM from my limited understanding of python - Vinod Kone On Sept. 24, 2018, 6:27 p.m., Andrew Schwartzmeyer wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68826/ > --- > > (Updated Sept. 24, 2018, 6:27 p.m.) > > > Review request for mesos, Benjamin Bannier and Vinod Kone. > > > Bugs: MESOS-9253 > https://issues.apache.org/jira/browse/MESOS-9253 > > > Repository: mesos > > > Description > --- > > Because Python is not type-safe, we encountered a bug in the code > executed on non-Windows platforms that was expecting `output` to be a > normal Python string instead of a Python byte string (with encoded > content). To fix this, we now always decode the bytes into a string, > so that the logic afterwards only has one type to deal with. > > > Diffs > - > > support/verify-reviews.py 72b7eb5b9baaf8eaa352b55dad55e62881d87323 > > > Diff: https://reviews.apache.org/r/68826/diff/1/ > > > Testing > --- > > > Thanks, > > Andrew Schwartzmeyer > >
Re: Review Request 68257: Fixed incorrect `mnt` namespace detection of command executor's task.
> On Aug. 7, 2018, 4:03 p.m., Vinod Kone wrote: > > src/slave/containerizer/mesos/utils.cpp > > Line 102 (original), 105 (patched) > > <https://reviews.apache.org/r/68257/diff/1/?file=2069850#file2069850line108> > > > > Are we guaranteed that there are no short-lived processes, other than > > the task process, at the 2nd level? If not, we will have the same issue > > right? > > > > Modulo the above question, the change LGTM. > > Andrei Budnik wrote: > There are two types of 2nd-level processes: > 1) the command executor's task > 2) the nested container's task > > E.g. the process tree can be like the following: > 0. mesos-containerizer (`init`) >1. mesos-executor (command executor) > 2. sleep 1000 (command executor's task) >1. mesos-containerizer (`init` of a nested container) <- enters `mnt` > namespace of command executor's task before forking a task > 2. echo "echo" (nested container's task) > > Since we skip 1st-level processes whose `mnt` namespace is not the same > as `init` process (PID 1), the algorithm doesn't iterate over their 2nd-level > processes. That gives a gurarantee that we only detect command executor's > task, but not the short-lived processes. > > Alexander Rukletsov wrote: > Let's make it explicit to the reader. I suggest to: > 1. Collect _candidates_ (2nd level children) first. > 2. Assert there are >= 1 candidates. > 3. Filter all candidates except those whose (`mnt` differs from root's > `mnt` && parent's `mnt` equals to root's `mnt`). > 4. Assert there are now 0 or 1 candidates left. > 5. Return pid of that single candidate or root's pid if there are none. @andrei Thanks for the info. IIUC, any processes launched by the mesos-executor or the task using `LAUNCH_NESTED_CONTAINER` API will end up with the process tree you showed above. I just wanted to make sure it is not possible for us to see a child process of command executor that is a sibling of the task process (e.g., task launched a process without using LNC API but it ended up as a sibling, command executor runs a script/binary without using LNC API etc). - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68257/#review206947 --- On Aug. 7, 2018, 1:46 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68257/ > --- > > (Updated Aug. 7, 2018, 1:46 p.m.) > > > Review request for mesos, Alexander Rukletsov, Gilbert Song, Jie Yu, and > Kevin Klues. > > > Bugs: MESOS-9116 > https://issues.apache.org/jira/browse/MESOS-9116 > > > Repository: mesos > > > Description > --- > > Previously, we were walking the process tree from the container's > `init` process to find the first process along the way whose `mnt` > namespace differs from the `init` process. We expected this algorithm > to always return the PID of the command executor's task. However, if > someone launches multiple nested containers within the process tree, > the algorithm might detect the PID of the nested container instead of > the command executor's task. The detected PID might belong to a > short-lived container, so the container's process might terminate at > the moment the containerizer launcher (aka `nanny`) process tries to > enter its `mnt` namespace. This patch fixes the detection algorithm > so that it always returns PID of the command executor's task. > > > Diffs > - > > src/slave/containerizer/mesos/utils.cpp > 30e76d1d91651975033078f5450e45f5f2fd8ba0 > > > Diff: https://reviews.apache.org/r/68257/diff/1/ > > > Testing > --- > > 1) Internal CI with disabled > `ROOT_CGROUPS_LaunchNestedContainerSessionsInParallel` test (see previous > patch). > 2) Fedora 25: `./src/mesos-tests > --gtest_filter=*AgentAPITest.LaunchNestedContainerSessionInParallel* > --gtest_break_on_failure --gtest_repeat=100 --verbose` > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68257: Fixed incorrect `mnt` namespace detection of command executor's task.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68257/#review206947 --- src/slave/containerizer/mesos/utils.cpp Line 102 (original), 105 (patched) <https://reviews.apache.org/r/68257/#comment290125> Are we guaranteed that there are no short-lived processes, other than the task process, at the 2nd level? If not, we will have the same issue right? Modulo the above question, the change LGTM. - Vinod Kone On Aug. 7, 2018, 1:46 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68257/ > --- > > (Updated Aug. 7, 2018, 1:46 p.m.) > > > Review request for mesos, Alexander Rukletsov, Gilbert Song, Jie Yu, and > Kevin Klues. > > > Bugs: MESOS-9116 > https://issues.apache.org/jira/browse/MESOS-9116 > > > Repository: mesos > > > Description > --- > > Previously, we were walking the process tree from the container's > `init` process to find the first process along the way whose `mnt` > namespace differs from the `init` process. We expected this algorithm > to always return the PID of the command executor's task. However, if > someone launches multiple nested containers within the process tree, > the algorithm might detect the PID of the nested container instead of > the command executor's task. The detected PID might belong to a > short-lived container, so the container's process might terminate at > the moment the containerizer launcher (aka `nanny`) process tries to > enter its `mnt` namespace. This patch fixes the detection algorithm > so that it always returns PID of the command executor's task. > > > Diffs > - > > src/slave/containerizer/mesos/utils.cpp > 30e76d1d91651975033078f5450e45f5f2fd8ba0 > > > Diff: https://reviews.apache.org/r/68257/diff/1/ > > > Testing > --- > > 1) Internal CI with disabled > `ROOT_CGROUPS_LaunchNestedContainerSessionsInParallel` test (see previous > patch). > 2) Fedora 25: `./src/mesos-tests > --gtest_filter=*AgentAPITest.LaunchNestedContainerSessionInParallel* > --gtest_break_on_failure --gtest_repeat=100 --verbose` > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68111: Added 'MesosCon 2018 CFP is now open!' blog post.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68111/#review206610 --- Fix it, then Ship it! site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md Lines 23 (patched) <https://reviews.apache.org/r/68111/#comment289640> s/2018/2017/ - Vinod Kone On July 30, 2018, 8:12 p.m., Gastón Kleiman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68111/ > --- > > (Updated July 30, 2018, 8:12 p.m.) > > > Review request for mesos, Greg Mann and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Added 'MesosCon 2018 CFP is now open!' blog post. > > > Diffs > - > > site/source/blog/2018-07-30-mesoscon-2018-cfp-is-now-open.md PRE-CREATION > > > Diff: https://reviews.apache.org/r/68111/diff/1/ > > > Testing > --- > > Used `./mesos-website-dev.sh` to verify that the website is properly rendered. > > > Thanks, > > Gastón Kleiman > >
Re: Review Request 67965: Optimized ranges subtraction operation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67965/#review206530 --- Ship it! Ship It! - Vinod Kone On July 26, 2018, 7:14 p.m., Meng Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67965/ > --- > > (Updated July 26, 2018, 7:14 p.m.) > > > Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Vinod Kone. > > > Bugs: MESOS-9086 > https://issues.apache.org/jira/browse/MESOS-9086 > > > Repository: mesos > > > Description > --- > > The current ranges subtraction uses boost IntervalSet. > Based on the profiling result of MESOS-8989, the ranges > subtraction operation is about 2~3 times more expensive > than that of addition. The conversion cost from Ranges > to IntervalSet may be the culprit. > > This patch optimizes the ranges subtraction operation by > converting Ranges to a vector of internal sub-ranges, sorting > the vector based on sub-range start and then applying a > one-pass algorithm similar to that of addition. > > > Diffs > - > > src/common/values.cpp afe4137f82115dd4ec9967b5eba16a1dd15401c8 > src/tests/values_tests.cpp 2f5abedfd461c114d03f5e941d81ebe414188033 > src/v1/values.cpp d2c31f6c91998382dec1d8834b40613013716cdd > > > Diff: https://reviews.apache.org/r/67965/diff/4/ > > > Testing > --- > > make check > > Benchmarking: > > tl:dr: more than 80% performance improvment across board. Performance of > subtraction is now on par or better than the addition, especially when there > are a large number of sub-ranges. > > Build with -O2 optimization, ran on a multicore machine with peak frequency > at 2.2GHz: > > Took 1.617706ms to perform 1000 'a += b' operations on ports:[1-6, 11-16, > 21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges > Took 1.607999ms to perform 1000 'a -= b' operations on ports:[1-6, 11-16, > 21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges > Took 3.094113ms to perform 1000 'a + b' operations on ports:[1-6, 11-16, > 21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges > Took 3.152291ms to perform 1000 'a - b' operations on ports:[1-6, 11-16, > 21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges > > Took 14.908924ms to perform 1000 'a += b' operations on ports:[1-6, 11-16, > 21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges > Took 13.564767ms to perform 1000 'a -= b' operations on ports:[1-6, 11-16, > 21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges > Took 25.523443ms to perform 1000 'a + b' operations on ports:[1-6, 11-16, > 21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges > Took 24.871216ms to perform 1000 'a - b' operations on ports:[1-6, 11-16, > 21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges > > Took 234.22483ms to perform 1000 'a += b' operations on ports:[1-6, 11-16, > 21-26...9991-9996] and ports:[3-8, 13-18, 23-28..., 9993-9998] with 1000 > sub-ranges > Took 144.417381ms to perform 1000 'a -= b' operations on ports:[1-6, 11-16, > 21-26...9991-9996] and ports:[3-8, 13-18, 23-28..., 9993-9998] with 1000 > sub-ranges > Took 322.548021ms to perform 1000 'a + b' operations on ports:[1-6, 11-16, > 21-26...9991-9996] and ports:[3-8, 13-18, 23-28..., 9993-9998] with 1000 > sub-ranges > Took 227.835441ms to perform 1000 'a - b' operations on ports:[1-6, 11-16, > 21-26...9991-9996] and ports:[3-8, 13-18, 23-28..., 9993-9998] with 1000 > sub-ranges > > > Thanks, > > Meng Zhu > >
Re: Review Request 67965: Optimized ranges subtraction operation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67965/#review206414 --- src/common/values.cpp Line 231 (original), 231 (patched) <https://reviews.apache.org/r/67965/#comment289353> extra blank line. src/common/values.cpp Lines 239 (patched) <https://reviews.apache.org/r/67965/#comment289354> `ranges` src/common/values.cpp Lines 263 (patched) <https://reviews.apache.org/r/67965/#comment289355> either use `iteratorLeft` and `iteratorRight` or `itLeft` and `itRight`. i've looked around and I don't think we use `itr*` anywhere. src/common/values.cpp Lines 265 (patched) <https://reviews.apache.org/r/67965/#comment289356> don't think you need the comment here. it's pretty clear? src/common/values.cpp Lines 290-311 (patched) <https://reviews.apache.org/r/67965/#comment289368> This part is a bit hard to follow. Can you break it down into 4 cases 1) left is subsumed in right 2) right is subsumed in left 3) left overlaps right but left's start is smaller 4) left overlaps right but right's start is smaller Even if some of these cases ends up with same resulting code, I think it's worth duplicating for readability. src/common/values.cpp Lines 313 (patched) <https://reviews.apache.org/r/67965/#comment289369> I like that you moved this outside the loop! src/common/values.cpp Line 371 (original), 464 (patched) <https://reviews.apache.org/r/67965/#comment289370> Can you add a TODO here to make this consistent with how we do subtraction? src/v1/values.cpp Lines 233 (patched) <https://reviews.apache.org/r/67965/#comment289371> See below. - Vinod Kone On July 24, 2018, 5:29 a.m., Meng Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67965/ > --- > > (Updated July 24, 2018, 5:29 a.m.) > > > Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Vinod Kone. > > > Bugs: MESOS-9086 > https://issues.apache.org/jira/browse/MESOS-9086 > > > Repository: mesos > > > Description > --- > > The current ranges subtraction uses boost IntervalSet. > Based on the profiling result of MESOS-8989, the ranges > subtraction operation is about 2~3 times more expensive > than that of addition. The conversion cost from Ranges > to IntervalSet may be the culprit. > > This patch optimizes the ranges subtraction operation by > converting Ranges to a vector of internal sub-ranges, sorting > the vector based on sub-range start and then applying a > one-pass algorithm similar to that of addition. > > > Diffs > - > > src/common/values.cpp afe4137f82115dd4ec9967b5eba16a1dd15401c8 > src/tests/values_tests.cpp 2f5abedfd461c114d03f5e941d81ebe414188033 > src/v1/values.cpp d2c31f6c91998382dec1d8834b40613013716cdd > > > Diff: https://reviews.apache.org/r/67965/diff/2/ > > > Testing > --- > > make check > > Benchmarking: > > tl:dr: more than 80% performance improvment across board. Performance of > subtraction is now on par or better than the addition, especially when there > are a large number of sub-ranges. > > Build with -O2 optimization, ran on a multicore machine with peak frequency > at 2.2GHz: > > Took 1.617706ms to perform 1000 'a += b' operations on ports:[1-6, 11-16, > 21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges > Took 1.607999ms to perform 1000 'a -= b' operations on ports:[1-6, 11-16, > 21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges > Took 3.094113ms to perform 1000 'a + b' operations on ports:[1-6, 11-16, > 21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges > Took 3.152291ms to perform 1000 'a - b' operations on ports:[1-6, 11-16, > 21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges > > Took 14.908924ms to perform 1000 'a += b' operations on ports:[1-6, 11-16, > 21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges > Took 13.564767ms to perform 1000 'a -= b' operations on ports:[1-6, 11-16, > 21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges > Took 25.523443ms to perform 1000 'a + b' operations on ports:[1-6, 11-16, > 21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges > Took 24.871216ms to perform 1000 'a - b' operations on ports:[1-6, 11-16, > 21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998]
