You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2015/06/18 03:02:45 UTC
Review Request 35587: Suppress task reconciliation status update
logging.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/
-----------------------------------------------------------
Review request for Aurora and Bill Farner.
Repository: aurora
Description
-------
Suppress task reconciliation status update logging.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
Diff: https://reviews.apache.org/r/35587/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/#review88325
-----------------------------------------------------------
Ship it!
Master (af7e1a7) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On June 18, 2015, 1:02 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35587/
> -----------------------------------------------------------
>
> (Updated June 18, 2015, 1:02 a.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Suppress task reconciliation status update logging.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
>
> Diff: https://reviews.apache.org/r/35587/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On June 18, 2015, 4:42 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java, line 314
> > <https://reviews.apache.org/r/35587/diff/1/?file=986660#file986660line314>
> >
> > What's being tested here? If i'm reading correctly, this test case would pass before the patch.
>
> Maxim Khutornenko wrote:
> This is mostly for test coverage similarly to the testOfferFirstAcceptsFineLogging above. Asserting a message would require replacing current anonymous logger with a mock and deeper refactoring. Do you think it's worth it here?
>
> Bill Farner wrote:
> You could pass a mock/stub `Logger` and expect the behavior (if you do, please be vague with expectations to avoid coupling to log message strings, etc). If that's not to your liking, you could punt.
Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/#review88388
-----------------------------------------------------------
On June 19, 2015, 10:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35587/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 10:07 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Suppress task reconciliation status update logging.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
>
> Diff: https://reviews.apache.org/r/35587/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On June 18, 2015, 4:42 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java, line 314
> > <https://reviews.apache.org/r/35587/diff/1/?file=986660#file986660line314>
> >
> > What's being tested here? If i'm reading correctly, this test case would pass before the patch.
This is mostly for test coverage similarly to the testOfferFirstAcceptsFineLogging above. Asserting a message would require replacing current anonymous logger with a mock and deeper refactoring. Do you think it's worth it here?
> On June 18, 2015, 4:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 198
> > <https://reviews.apache.org/r/35587/diff/1/?file=986659#file986659line198>
> >
> > A bit more context will help future readers. You could mention that reconciliation is expected to result in many no-op status updates.
Sure, done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/#review88388
-----------------------------------------------------------
On June 18, 2015, 1:02 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35587/
> -----------------------------------------------------------
>
> (Updated June 18, 2015, 1:02 a.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Suppress task reconciliation status update logging.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
>
> Diff: https://reviews.apache.org/r/35587/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Bill Farner <wf...@apache.org>.
> On June 18, 2015, 4:42 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java, line 314
> > <https://reviews.apache.org/r/35587/diff/1/?file=986660#file986660line314>
> >
> > What's being tested here? If i'm reading correctly, this test case would pass before the patch.
>
> Maxim Khutornenko wrote:
> This is mostly for test coverage similarly to the testOfferFirstAcceptsFineLogging above. Asserting a message would require replacing current anonymous logger with a mock and deeper refactoring. Do you think it's worth it here?
You could pass a mock/stub `Logger` and expect the behavior (if you do, please be vague with expectations to avoid coupling to log message strings, etc). If that's not to your liking, you could punt.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/#review88388
-----------------------------------------------------------
On June 18, 2015, 5:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35587/
> -----------------------------------------------------------
>
> (Updated June 18, 2015, 5:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Suppress task reconciliation status update logging.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
>
> Diff: https://reviews.apache.org/r/35587/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/#review88388
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java (line 198)
<https://reviews.apache.org/r/35587/#comment140903>
A bit more context will help future readers. You could mention that reconciliation is expected to result in many no-op status updates.
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java (line 314)
<https://reviews.apache.org/r/35587/#comment140904>
What's being tested here? If i'm reading correctly, this test case would pass before the patch.
- Bill Farner
On June 18, 2015, 1:02 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35587/
> -----------------------------------------------------------
>
> (Updated June 18, 2015, 1:02 a.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Suppress task reconciliation status update logging.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
>
> Diff: https://reviews.apache.org/r/35587/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/#review88806
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On June 19, 2015, 10:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35587/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 10:07 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Suppress task reconciliation status update logging.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
>
> Diff: https://reviews.apache.org/r/35587/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/#review88604
-----------------------------------------------------------
Ship it!
Master (d240926) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On June 19, 2015, 10:07 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35587/
> -----------------------------------------------------------
>
> (Updated June 19, 2015, 10:07 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Suppress task reconciliation status update logging.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
>
> Diff: https://reviews.apache.org/r/35587/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/
-----------------------------------------------------------
(Updated June 19, 2015, 10:07 p.m.)
Review request for Aurora and Bill Farner.
Changes
-------
Bill's comments.
Repository: aurora
Description
-------
Suppress task reconciliation status update logging.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
Diff: https://reviews.apache.org/r/35587/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/#review88415
-----------------------------------------------------------
Ship it!
Master (af7e1a7) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On June 18, 2015, 5:04 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35587/
> -----------------------------------------------------------
>
> (Updated June 18, 2015, 5:04 p.m.)
>
>
> Review request for Aurora and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Suppress task reconciliation status update logging.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
> src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
>
> Diff: https://reviews.apache.org/r/35587/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 35587: Suppress task reconciliation status update
logging.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35587/
-----------------------------------------------------------
(Updated June 18, 2015, 5:04 p.m.)
Review request for Aurora and Bill Farner.
Changes
-------
Bill's comments.
Repository: aurora
Description
-------
Suppress task reconciliation status update logging.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java f233d5a181bb1f43fbbfe657dbda2cf975aa6895
src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java f0f9ac392973a276028aee3e06517a6e6d960bb6
Diff: https://reviews.apache.org/r/35587/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Maxim Khutornenko