mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 54183: Improved management of unreachable and completed tasks in master.
Date Tue, 10 Jan 2017 23:50:08 GMT


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5496
> > <https://reviews.apache.org/r/54183/diff/6/?file=1588710#file1588710line5496>
> >
> >     it's not necessary that the agent has failed over if we are here right?
> 
> Neil Conway wrote:
>     Do you mean "it's not necessary that the master has failed over if we are here"?
>     
>     If so, that's right: the master may or may not have re-registered in the time since
the agent was marked unreachable. The comment talks about how these two cases are handled
differently.

I forgot what I was asking here originally :( Lets drop it for now.


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.hpp, lines 2634-2641
> > <https://reviews.apache.org/r/54183/diff/6/?file=1588709#file1588709line2634>
> >
> >     this reads like `unreachableTasks` are completed tasks of PA frameworks. can
you split this comment up and move the unreachable specific comments to #2644?
> 
> Neil Conway wrote:
>     I added an additional comment describing the purpose of `unreachableTasks`, but I
felt like it was useful to still keep a note in the `completedTasks` comment to describe which
tasks are stored in `unreachableTasks` vs `completedTasks` when an agent is marked unreachable.
Let me know if you think this is still confusing.

lgtm.


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5507
> > <https://reviews.apache.org/r/54183/diff/6/?file=1588710#file1588710line5507>
> >
> >     can we inline this?
> 
> Neil Conway wrote:
>     We could, but personally I find it more readable to make it a separate function.
The logic is a bit involved here (since we need to check two different data structures due
to backward compatibility requirements), and there's already a lot going on in `Master::_reregisterSlave`.

In general, we tend to factor out something into a function if it is reusable and generic,
not to reduce the number of lines someone has to read in a function. In fact the argument
was to inline in such cases because it will avoid the need to context switch. 

Both `findPartitonAwareFrameworks` and `filterPartitionTasks` seem very specific to the `_reregisterSlave`
method, hence the suggestion.

For example, instead of looping through all the agent's tasks 3 times to figure out which
tasks to add to `Slave*`, it's probably easier to do it in one inline loop here?


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5523
> > <https://reviews.apache.org/r/54183/diff/6/?file=1588710#file1588710line5523>
> >
> >     not sure if this is worth making into a function?
> 
> Neil Conway wrote:
>     I thought it was a bit more readable (against, `Master::_reregisterSlave` has a lot
of stuff going on), but happy to change it if you disagree.

see above.


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 5539
> > <https://reviews.apache.org/r/54183/diff/6/?file=1588710#file1588710line5539>
> >
> >     what if the agent was marked unreachable by the previous master? do we still
want to add all its tasks?
> 
> Neil Conway wrote:
>     If the master has failed over, all tasks on the agent will be allowed to continue
running (whether partition-aware or not) -- so that's why we re-add all tasks here.

I see.


> On Jan. 3, 2017, 10:25 p.m., Vinod Kone wrote:
> > src/tests/partition_tests.cpp, lines 1819-1847
> > <https://reviews.apache.org/r/54183/diff/6/?file=1588711#file1588711line1819>
> >
> >     hmm. it is unfortunate that we send TASK_LOST followed by TASK_FINISHED and
then send TASK_LOST on reconciliation. can you remind me why the master forwards status updates
for unknown tasks? looks like it can just drop them if the reason for doing so is no longer
valid.
> 
> Neil Conway wrote:
>     I don't know why we forward status updates for unknown tasks. I can see it having
some value, though -- in the scenario above, the first `TASK_LOST` just means the agent become
unreachable. `TASK_FINISHED` tells the framework that the task actually did complete successfully.
I can imagine situations where, say, the framework ignores `TASK_LOST` or uses a large timeout
before assuming the task is "really lost", whereas `TASK_FINISHED` / `TASK_KILLED` / etc.
is a more definitive signal.

I see. Atleat the PA world is much saner. TASK_UNREACHABLE -> TASK_FINISHED -> TASK_UNKNOWN


- Vinod


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


On Jan. 9, 2017, 10:39 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54183/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2017, 10:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6619
>     https://issues.apache.org/jira/browse/MESOS-6619
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Before partition-awareness, when an agent failed health checks, the
> master removed the agent from the registry, marked all of its tasks
> TASK_LOST, and moved them to the `completedTasks` list in the master's
> memory. Although "lost" tasks might still be running, partitioned agents
> would only be allowed to re-register if the master failed over, in which
> case the `completedTasks` map would be emptied.
> 
> When partition-awareness was introduced, we initially followed the same
> scheme, with the only difference that partition-aware tasks are marked
> TASK_UNREACHABLE, not TASK_LOST.
> 
> This scheme has a few shortcomings. First, partition-aware tasks might
> resume running when the partitioned agent re-registers. Second, we
> re-added non-partition aware tasks when the agent re-registered but then
> marked them completed when the framework is shutdown, resulting in two
> entries in `completedTasks`.
> 
> This commit introduces a separate bounded map, `unreachableTasks`. These
> tasks are reported separately via the HTTP endpoints, because they have
> different semantics (unlike completed tasks, unreachable tasks can
> resume running). The size of this map is limited by a new master flag,
> `--max_unreachable_tasks_per_framework`. This commit also changes the
> master to omit re-adding non-partition-aware tasks on re-registering
> agents (unless the master has failed over): those tasks will shortly be
> shutdown anyway.
> 
> Finally, this commit fixes a minor bug in the previous code: the
> previous coding neglected to shutdown non-partition-aware frameworks
> running on pre-1.0 Mesos agents that re-register with the master after
> a network partition.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 2113d06f58dddc0a28ae1241a24096266fe39801 
>   include/mesos/master/master.proto 03203c8ddd05876339b1c56789b7bb140499d49d 
>   include/mesos/v1/master/master.proto f8edf39a68752c8601cece345f52bce8b9a8a68b 
>   src/master/constants.hpp 900b69460de9280f30b16b86925e436054e080ca 
>   src/master/flags.hpp 6a17b763dc76daa10073394f416b049e97a44238 
>   src/master/flags.cpp 124bfbca6be9d5d60597b9a1793bd807202da8ec 
>   src/master/http.cpp 75dcd6199dbfcca6260baed49b838a45312bb667 
>   src/master/master.hpp 368ee1d5e97784fa54e0f141906405ee8f104317 
>   src/master/master.cpp 0c2210bbcd0622f704497daf78fe959cde99ff7f 
>   src/tests/partition_tests.cpp 72013d1bfee275c6f3cb90173f0c408d55e0bc5d 
> 
> Diff: https://reviews.apache.org/r/54183/diff/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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