Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73603 --- Ship it! Ship It! - Joshua Cohen On Feb. 21, 2015, 6:33 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 6:33 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 21, 2015, 12:53 a.m., Joshua Cohen wrote: Can you fill in testing done? Bill Farner wrote: Honest question - do you find that useful for changes like this? I find it redundant to always type `./gradlew build -Pq`, especially since the build bot will do that anyhow. My take is that, yes, I trust that you ran the tests. But for someone who's pushing a review for the first time, who knows? And if they just pick a random commit from the log and look at the Testing Done section, I'd like for it to be consistent and easy to emulate. On Feb. 21, 2015, 12:53 a.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, lines 199-200 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line199 Use StringBuilder and String.format? Bill Farner wrote: See my reply to Maxim's comment. javac is smart about using StringBuilder behind the scenes in cases like this. Do you find String.format more readable in cases like this? Personally i do not. I've never had a problem with multiple concatenated variables, but my main concern is that we're consistent. My interpretation of our style based on review feedback I've gotten is that when concatenating more than one variable into a string we err on the side of using String.format for readability (perhaps we should actually fork the twitter style guide and codify these things?). - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73353 --- On Feb. 21, 2015, 6:33 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 6:33 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 23, 2015, 6:22 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, lines 199-201 https://reviews.apache.org/r/31248/diff/5/?file=871734#file871734line199 Well, I think this is actually worse than it was. In case there is no StringBuilder concatenation active in JVM, this is still producing unnecessary string objects. However, in the opposite case, there will be muiltiple StringBuilder objects and heap churn may be even worse. Don't want to block review on this anymore but please, convert everything to a single StringBuilder without concatenation or revert back to concatenation to hopefully take advantage of an auto generated StringBuilder. Sorry, that was hasty, and i agree an odd use of StringBuilder. I will update to remove concatenation. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73608 --- On Feb. 21, 2015, 6:33 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 6:33 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 23, 2015, 8:02 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, lines 204-205 https://reviews.apache.org/r/31248/diff/5-6/?file=871734#file871734line204 one line I'm indifferent, figured whichever line break i chose i'd get a nit for the opposite. Changed. On Feb. 23, 2015, 8:02 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, lines 208-209 https://reviews.apache.org/r/31248/diff/5-6/?file=871734#file871734line208 one line Ditto. On Feb. 23, 2015, 8:02 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, lines 212-213 https://reviews.apache.org/r/31248/diff/5-6/?file=871734#file871734line212 one line Ditto. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73649 --- On Feb. 23, 2015, 7:03 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 23, 2015, 7:03 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 23, 2015, 10:34 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 21, 2015, 12:39 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 199 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line199 Use StringBuilder instead to avoid heap churn. Bill Farner wrote: javac is smart enough here http://stackoverflow.com/questions/11942368/why-use-stringbuilder-explicitly-if-the-compiler-converts-string-concatenation-t Maxim Khutornenko wrote: This is not obvious and may be dependent on particular JVM complier version/options. I'd rather go with an explicit approach than second guess JVM internals. Bill Farner wrote: Mind if we wait to observe a problem? This seems like premature optimization in a relatively infrequent code path, i'd rather favor readabilty. Maxim Khutornenko wrote: How do you propose we observe the problem? :) It's all these unaccounted/non-obvious little fragments that may contribute to the churn. Since it logs on every task status update unconditionally, I'd rather eliminate the potential problem than deal with possible consequences. Bill Farner wrote: Stepping back, String's javadoc actually documents this behavior [1] The Java language provides special support for the string concatenation operator ( + ), and for conversion of other objects to strings. String concatenation is implemented through the StringBuilder(or StringBuffer) class and its append method. String conversions are implemented through the method toString, defined by Object and inherited by all classes in Java. For additional information on string concatenation and conversion, see Gosling, Joy, and Steele, The Java Language Specification. [1] http://docs.oracle.com/javase/7/docs/api/java/lang/String.html Maxim Khutornenko wrote: This is better. Though specification is still non-assertive about it: To increase the performance of repeated string concatenation, a Java compiler **may use** the StringBuffer class or a similar technique to reduce the number of intermediate String objects that are created by evaluation of an expression. http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.18.1 Ok, i'll fold. I suppose there's little harm in this case. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73351 --- On Feb. 21, 2015, 1:36 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:36 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 6:33 p.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 21, 2015, 12:39 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java, line 78 https://reviews.apache.org/r/31248/diff/1/?file=871337#file871337line78 Any reason to lower case these? I'd actually prefer upper cased (default) values. This would be consistent with other places where we use enums (e.g. task_store_ASSIGNED). Yeah, not sure why i did that. Done. On Feb. 21, 2015, 12:39 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 216 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line216 Should it rather be micros (given that the only current consumer accepts microseconds)? Sure. Also added a guard at the receiving side against =0 delta in case of 1ms delivery or clock skew. On Feb. 21, 2015, 12:39 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 199 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line199 Use StringBuilder instead to avoid heap churn. javac is smart enough here http://stackoverflow.com/questions/11942368/why-use-stringbuilder-explicitly-if-the-compiler-converts-string-concatenation-t - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73351 --- On Feb. 21, 2015, 12:04 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 12:04 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:36 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73372 --- Ship it! Master (e5de618) 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 Feb. 21, 2015, 1:36 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:36 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 21, 2015, 12:39 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 199 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line199 Use StringBuilder instead to avoid heap churn. Bill Farner wrote: javac is smart enough here http://stackoverflow.com/questions/11942368/why-use-stringbuilder-explicitly-if-the-compiler-converts-string-concatenation-t Maxim Khutornenko wrote: This is not obvious and may be dependent on particular JVM complier version/options. I'd rather go with an explicit approach than second guess JVM internals. Bill Farner wrote: Mind if we wait to observe a problem? This seems like premature optimization in a relatively infrequent code path, i'd rather favor readabilty. Maxim Khutornenko wrote: How do you propose we observe the problem? :) It's all these unaccounted/non-obvious little fragments that may contribute to the churn. Since it logs on every task status update unconditionally, I'd rather eliminate the potential problem than deal with possible consequences. Bill Farner wrote: Stepping back, String's javadoc actually documents this behavior [1] The Java language provides special support for the string concatenation operator ( + ), and for conversion of other objects to strings. String concatenation is implemented through the StringBuilder(or StringBuffer) class and its append method. String conversions are implemented through the method toString, defined by Object and inherited by all classes in Java. For additional information on string concatenation and conversion, see Gosling, Joy, and Steele, The Java Language Specification. [1] http://docs.oracle.com/javase/7/docs/api/java/lang/String.html This is better. Though specification is still non-assertive about it: To increase the performance of repeated string concatenation, a Java compiler **may use** the StringBuffer class or a similar technique to reduce the number of intermediate String objects that are created by evaluation of an expression. http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.18.1 - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73351 --- On Feb. 21, 2015, 1:36 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:36 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:11 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:34 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 21, 2015, 12:39 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 199 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line199 Use StringBuilder instead to avoid heap churn. Bill Farner wrote: javac is smart enough here http://stackoverflow.com/questions/11942368/why-use-stringbuilder-explicitly-if-the-compiler-converts-string-concatenation-t Maxim Khutornenko wrote: This is not obvious and may be dependent on particular JVM complier version/options. I'd rather go with an explicit approach than second guess JVM internals. Bill Farner wrote: Mind if we wait to observe a problem? This seems like premature optimization in a relatively infrequent code path, i'd rather favor readabilty. How do you propose we observe the problem? :) It's all these unaccounted/non-obvious little fragments that may contribute to the churn. Since it logs on every task status update unconditionally, I'd rather eliminate the potential problem than deal with possible consequences. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73351 --- On Feb. 21, 2015, 1:36 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:36 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 21, 2015, 12:53 a.m., Joshua Cohen wrote: Can you fill in testing done? Honest question - do you find that useful for changes like this? I find it redundant to always type `./gradlew build -Pq`, especially since the build bot will do that anyhow. On Feb. 21, 2015, 12:53 a.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, lines 199-200 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line199 Use StringBuilder and String.format? See my reply to Maxim's comment. javac is smart about using StringBuilder behind the scenes in cases like this. Do you find String.format more readable in cases like this? Personally i do not. On Feb. 21, 2015, 12:53 a.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java, line 44 https://reviews.apache.org/r/31248/diff/1/?file=871337#file871337line44 s/loast/lost Fixed. On Feb. 21, 2015, 12:53 a.m., Joshua Cohen wrote: src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java, line 85 https://reviews.apache.org/r/31248/diff/1/?file=871339#file871339line85 move these to previous line (here, elsewhere)? Done. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73353 --- On Feb. 21, 2015, 1:11 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:11 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 21, 2015, 12:39 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 199 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line199 Use StringBuilder instead to avoid heap churn. Bill Farner wrote: javac is smart enough here http://stackoverflow.com/questions/11942368/why-use-stringbuilder-explicitly-if-the-compiler-converts-string-concatenation-t Maxim Khutornenko wrote: This is not obvious and may be dependent on particular JVM complier version/options. I'd rather go with an explicit approach than second guess JVM internals. Bill Farner wrote: Mind if we wait to observe a problem? This seems like premature optimization in a relatively infrequent code path, i'd rather favor readabilty. Maxim Khutornenko wrote: How do you propose we observe the problem? :) It's all these unaccounted/non-obvious little fragments that may contribute to the churn. Since it logs on every task status update unconditionally, I'd rather eliminate the potential problem than deal with possible consequences. Stepping back, String's javadoc actually documents this behavior [1] The Java language provides special support for the string concatenation operator ( + ), and for conversion of other objects to strings. String concatenation is implemented through the StringBuilder(or StringBuffer) class and its append method. String conversions are implemented through the method toString, defined by Object and inherited by all classes in Java. For additional information on string concatenation and conversion, see Gosling, Joy, and Steele, The Java Language Specification. [1] http://docs.oracle.com/javase/7/docs/api/java/lang/String.html - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73351 --- On Feb. 21, 2015, 1:36 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:36 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73351 --- src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java https://reviews.apache.org/r/31248/#comment119647 Use StringBuilder instead to avoid heap churn. src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java https://reviews.apache.org/r/31248/#comment119648 Should it rather be micros (given that the only current consumer accepts microseconds)? src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java https://reviews.apache.org/r/31248/#comment119646 Any reason to lower case these? I'd actually prefer upper cased (default) values. This would be consistent with other places where we use enums (e.g. task_store_ASSIGNED). - Maxim Khutornenko On Feb. 21, 2015, 12:04 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 12:04 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 31248: Export stats for source and reason for LOST tasks, and status delivery latency.
On Feb. 21, 2015, 12:39 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java, line 199 https://reviews.apache.org/r/31248/diff/1/?file=871335#file871335line199 Use StringBuilder instead to avoid heap churn. Bill Farner wrote: javac is smart enough here http://stackoverflow.com/questions/11942368/why-use-stringbuilder-explicitly-if-the-compiler-converts-string-concatenation-t This is not obvious and may be dependent on particular JVM complier version/options. I'd rather go with an explicit approach than second guess JVM internals. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/#review73351 --- On Feb. 21, 2015, 1:11 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31248/ --- (Updated Feb. 21, 2015, 1:11 a.m.) Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-1028 https://issues.apache.org/jira/browse/AURORA-1028 Repository: aurora Description --- Export stats for source and reason for LOST tasks, and status delivery latency. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 1d8f0128732756db74576ee669f6a2718fecc105 src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java ffc30bb548706df7bec9e1502242890e9b5eb942 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 59ad9e65589c421cefb76f265446fa2885e6198c src/main/java/org/apache/aurora/scheduler/mesos/TaskStatusStats.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java d02c6b32841d5d39c5780e7a044079a38729effb src/test/java/org/apache/aurora/scheduler/mesos/TaskStatusStatsTest.java PRE-CREATION Diff: https://reviews.apache.org/r/31248/diff/ Testing --- Thanks, Bill Farner
