mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 69452: Added a test for executors sending messages to recovered frameworks.
Date Tue, 27 Nov 2018 21:32:36 GMT


> On Nov. 27, 2018, 9:16 p.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp
> > Lines 3850 (patched)
> > <https://reviews.apache.org/r/69452/diff/1/?file=2110508#file2110508line3850>
> >
> >     Maybe a TODO to simplify this test by having a test scheduler that knows how
to launch a mock executor?

I'm not sure if this is a good idea, because the default (composing or mesos containerizer)
would launch the executor in another linux process, which would be impossible for mocking,
unless we have another binary that just forward all the requests back to the mock executor
in the test process. Using the test containerizer seems more simpler.


> On Nov. 27, 2018, 9:16 p.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp
> > Lines 3861 (patched)
> > <https://reviews.apache.org/r/69452/diff/1/?file=2110508#file2110508line3861>
> >
> >     executorConnected?

This holds the `executor::Mesos*` object that we'll use later, and the `...Library` name is
consistent to other tests where we name the object.


> On Nov. 27, 2018, 9:16 p.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp
> > Lines 3974-3976 (patched)
> > <https://reviews.apache.org/r/69452/diff/1/?file=2110508#file2110508line3974>
> >
> >     Maybe settle at the end anyway, so that we start calling destructors after the
message finishes processing? Right now it will start after the message starts processing.

Do you mean pausing the clock and do a `Clock::settle()` before the test finishes? Yeah right
now the teardown will start after the message starts processing, but the teardown process
would wait for the master process to terminate.


> On Nov. 27, 2018, 9:16 p.m., Benjamin Mahler wrote:
> > src/tests/mesos.hpp
> > Line 907 (original), 919 (patched)
> > <https://reviews.apache.org/r/69452/diff/1/?file=2110509#file2110509line919>
> >
> >     just curious, what happened here? seems like this should be a different patch?

Sure let me split it into another patch. It's just that the common helper uses the `slave_id`
accesser, which is not universal to v0/v1.


- Chun-Hung


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69452/#review210905
-----------------------------------------------------------


On Nov. 27, 2018, 4:58 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69452/
> -----------------------------------------------------------
> 
> (Updated Nov. 27, 2018, 4:58 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9419
>     https://issues.apache.org/jira/browse/MESOS-9419
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds the `ExecutorMessageToRecoveredHttpFramework` test to
> ensure that a master will not crash when forwarding an executor message
> to a recovered framework to prevent any future regression of MESOS-9419.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_tests.cpp 651bb9ba9298fc3d7179ed86487aa55be519ce12 
>   src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e 
> 
> 
> Diff: https://reviews.apache.org/r/69452/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> This test will fail without r/69451.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message