You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2017/08/30 14:14:44 UTC
Review Request 61991: Added several logs to the C++ part of the v1-v0
adapter.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
Diff: https://reviews.apache.org/r/61991/diff/1/
Testing
-------
None: Not a functional change.
Thanks,
Alexander Rukletsov
Re: Review Request 61991: Added several logs to the C++ part of the
v1-v0 adapter.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/#review184162
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61579, 61580, 61991]
Logs available here: http://104.210.40.105/logs/master/61991
- Mesos Reviewbot Windows
On Aug. 30, 2017, 2:14 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61991/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 2:14 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
>
>
> Diff: https://reviews.apache.org/r/61991/diff/1/
>
>
> Testing
> -------
>
> None: Not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 61991: Added several logs to the C++ part of the
v1-v0 adapter.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/#review184338
-----------------------------------------------------------
Fix it, then Ship it!
LGTM
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
Lines 360 (patched)
<https://reviews.apache.org/r/61991/#comment260474>
s/connecting/invoking
s/scheduler/connected callback
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
Lines 419 (patched)
<https://reviews.apache.org/r/61991/#comment260475>
How about:
"Disconnected with the Mesos master; invoking the disconnected callback" to be consistent with the registered logging message?
- Anand Mazumdar
On Aug. 31, 2017, 7:09 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61991/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 7:09 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
>
>
> Diff: https://reviews.apache.org/r/61991/diff/2/
>
>
> Testing
> -------
>
> None: Not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 61991: Added several logs to the C++ part of the
v1-v0 adapter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/
-----------------------------------------------------------
(Updated Sept. 1, 2017, 8:36 a.m.)
Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
Bugs: MESOS-7872
https://issues.apache.org/jira/browse/MESOS-7872
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
Diff: https://reviews.apache.org/r/61991/diff/2/
Testing
-------
None: Not a functional change.
Thanks,
Alexander Rukletsov
Re: Review Request 61991: Added several logs to the C++ part of the
v1-v0 adapter.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/#review184251
-----------------------------------------------------------
Bad patch!
Reviews applied: [61579, 61580, 61991]
Logs available here: http://104.210.40.105/logs/master/61991
- Mesos Reviewbot Windows
On Aug. 31, 2017, 12:09 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61991/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 12:09 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
>
>
> Diff: https://reviews.apache.org/r/61991/diff/2/
>
>
> Testing
> -------
>
> None: Not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 61991: Added several logs to the C++ part of the
v1-v0 adapter.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/#review184256
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61579, 61580, 61991]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Aug. 31, 2017, 7:09 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61991/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 7:09 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
>
>
> Diff: https://reviews.apache.org/r/61991/diff/2/
>
>
> Testing
> -------
>
> None: Not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 61991: Added several logs to the C++ part of the
v1-v0 adapter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/
-----------------------------------------------------------
(Updated Aug. 31, 2017, 7:09 a.m.)
Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
Diff: https://reviews.apache.org/r/61991/diff/2/
Changes: https://reviews.apache.org/r/61991/diff/1-2/
Testing
-------
None: Not a functional change.
Thanks,
Alexander Rukletsov
Re: Review Request 61991: Added several logs to the C++ part of the
v1-v0 adapter.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 30, 2017, 9:45 p.m., Anand Mazumdar wrote:
> > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
> > Lines 360 (patched)
> > <https://reviews.apache.org/r/61991/diff/1/?file=1807837#file1807837line360>
> >
> > hmm, I think we should only log things that are relevant to the business logic of the adapter itself and different than the already existing logic of the driver it already wraps.
> >
> > In this case, this should already be logged by the v0 driver implementation. Hence, I don't see much utility in double logging it again here?
I think there is value in this log entry for two reasons:
1. The driver logs the `registered` event, while we log here that we are about to send the `connected` event.
2. The presence of the log entry makes it clear that there is an adapter behind the scenes and how it works.
Since it is not a periodical event, I would argue adding this log entry adds value.
> On Aug. 30, 2017, 9:45 p.m., Anand Mazumdar wrote:
> > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
> > Lines 408 (patched)
> > <https://reviews.apache.org/r/61991/diff/1/?file=1807837#file1807837line408>
> >
> > We should certainly log this as this is pertaining to the business logic of the adapter itself.
> >
> >
> > Also mention the reason i.e., we are dropping these pending events due to a master disconnection?
Sure.
> On Aug. 30, 2017, 9:45 p.m., Anand Mazumdar wrote:
> > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
> > Lines 418 (patched)
> > <https://reviews.apache.org/r/61991/diff/1/?file=1807837#file1807837line418>
> >
> > Ditto as above. We shouldn't double log here.
Why double log? IIUC, `SchedulerProcess::detected()` does not log before disconnecting the scheduler. Moreover, even if it would, I'm convinced there is value in this log entry to show the reader how the adapter reacted to the disconnected event because connected-disconnected and registered/disconnected logics are not symmetrical.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/#review184205
-----------------------------------------------------------
On Aug. 30, 2017, 2:14 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61991/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 2:14 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
>
>
> Diff: https://reviews.apache.org/r/61991/diff/1/
>
>
> Testing
> -------
>
> None: Not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 61991: Added several logs to the C++ part of the
v1-v0 adapter.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61991/#review184205
-----------------------------------------------------------
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
Lines 360 (patched)
<https://reviews.apache.org/r/61991/#comment260273>
hmm, I think we should only log things that are relevant to the business logic of the adapter itself and different than the already existing logic of the driver it already wraps.
In this case, this should already be logged by the v0 driver implementation. Hence, I don't see much utility in double logging it again here?
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
Lines 408 (patched)
<https://reviews.apache.org/r/61991/#comment260275>
We should certainly log this as this is pertaining to the business logic of the adapter itself.
Also mention the reason i.e., we are dropping these pending events due to a master disconnection?
src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
Lines 418 (patched)
<https://reviews.apache.org/r/61991/#comment260274>
Ditto as above. We shouldn't double log here.
- Anand Mazumdar
On Aug. 30, 2017, 2:14 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61991/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2017, 2:14 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Till Toenshoff, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 1f58fbff4e8414e4d2ae4c8f69b637ee3315e411
>
>
> Diff: https://reviews.apache.org/r/61991/diff/1/
>
>
> Testing
> -------
>
> None: Not a functional change.
>
>
> Thanks,
>
> Alexander Rukletsov
>
>