[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 giving up: https://github.com/palindromicity/stackwatch ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 @garydgregory, since we are getting out of code review / pr and into bigger issues, can we move your comments to jira? Gilles doesn't have github and the conversation will make more sense if we are all there. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user garydgregory commented on the issue:
https://github.com/apache/commons-lang/pull/311
When I see code like
```
watch.visit(new StackWatch.TimingVisitor() {
@Override
public void visitTiming(int level, List path,
StackWatch.Timing node) {
assertTrue(node.getStopWatch().getNanoTime() >
stopWatch.getNanoTime());
}
});
```
I have my doubts as to this belonging in Commons Lang and not a new module
like a Commons Time (but nothing with Joda-Time or java.time overlap) or
Commons Perf.
This feels very 'framework-y' to me and beyond the otherwise simple Commons
Lang APIs.
Thoughts from anyone else?
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 @garydgregory @kinow I added the static method. I did not squash/rebase though. I will if you need me to but I want the review. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
same with the static factory:
```java
@Test
public void testTryWithResourcesStaticFactory() {
final StopWatch stopWatch = new StopWatch();
try(StackWatch watch =
StackWatch.startStackWatch("testStackWatch")) {
stopWatch.start();
stopWatch.stop();
}
watch.visit(new StackWatch.TimingVisitor() {
@Override
public void visitTiming(int level, List path,
StackWatch.Timing node) {
assertTrue(node.getStopWatch().getNanoTime() >
stopWatch.getNanoTime());
}
});
}
```
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
the static factory, is pretty straight forward.
The try with resources, less so. What ends up happening is you cannot stop
the watch before visiting the nodes.
For example, this is what you want to do if you are going to time, but
visit later:
```java
@Test
public void testTryWithResources() {
final StopWatch stopWatch = new StopWatch();
try(StackWatch watch = new
StackWatch<>("testStackWatch")) {
watch.start();
stopWatch.start();
stopWatch.stop();
}
watch.visit(new StackWatch.TimingVisitor() {
@Override
public void visitTiming(int level, List path,
StackWatch.Timing node) {
assertTrue(node.getStopWatch().getNanoTime() >
stopWatch.getNanoTime());
}
});
}
```
but it won't work/compile with try with resources
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 Maybe there should be a commons-examples project with the src for examples? The java doc examples could come from there ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 To do try with resources we would have to have do nothing default returns right? ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 Sorry @garydgregory I understood you to mean it would be optional. So, the context of the creation of this idea is I did it for an environment where we have optional available 'capabilities', and it is common to check and use if present or fail if required but not present. There is a difference between the logging example and this. I am familiar with logging quick fail ( I implemented my own c++ streams based quick failing logger back in the day even ), but the logger is always there, you are checking for configuration or optionally enabled functionality ( debug logging ), where as I wrote this for an environment where we may not have a watch at all. I am not saying having a 'singleton' watch, with a 'do nothing' set of default interface implementations isn't something to think about. But things start from where you have a need usually. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user garydgregory commented on the issue: https://github.com/apache/commons-lang/pull/311 WRT "Try on the overall watch would have to behave differently, as it would both close the watch and stop the root timing..." I am not saying you would always use a try-with-resources, just that it should be an option. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user garydgregory commented on the issue:
https://github.com/apache/commons-lang/pull/311
I hope we do not advocate running code like the fragment you just quoted.
This is the same kind of issue we finally resolved in Apache Log4 2 where you
no longer need to write code like:
```
if (logger.isDebugEnabled()) {
logger.debug("This is " + foo + " and " +bar);
}
```
Instead you say:
`logger.debug("This is {} and {}", foo, bar);`
You can also delegate evaluation of expensive calls only if the logger is
enabled:
`logger.debug("This is {} and {}", () -> foo(), () -> bar());`
The logger is configured from a file or another code snippet.
IMO, we are already dangerously close to out of bounds of [lang] with the
current code, so I am mentioning this for exploration ATM. That said, I think
it would be good to see in text at least what a fully featured gadget looks
like before we include the current version in [lang]. Once in, it will likely
evolve, and then it might end up being moved to another component if it is no
longer 'lang-y'...
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 Try on the overall watch would have to behave differently, as it would both close the watch and stop the root timing... ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
```java
Optional watchOptional = state.context.getWatch();
watchOptional.ifPresent((sw) -> sw.startTiming("lambda", "LAMBDA"));
Object result = apply(localState);
watchOptional.ifPresent(StackWatch::stopTiming);
return result;
```
```java
Optional watchOptional = state.context.getWatch();
Object result;
if(watchOption.isPresent() {
try( TimingNode timing =
watchOption.get().startTiming("lambda","LAMBDA")) {
result = apply(localState);
}
} else {
result = apply(localState);
}
return result;
```
or something
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 That doesn't mean I don't want to re-visit the idea ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue:
https://github.com/apache/commons-lang/pull/311
I did think of that at the start. There were two issues for me:
1. I didn't want to expose the internal node to the caller and complicate
the interface
2. In my scenario ( a language engine, that *might* be timing things in the
repl or *might not* be timing things at runtime in a storm topology, there may
not be a watch at all, so having:
if(context.WatchOptional.isPresent())(
try(xx){
TIMED_FUNCTION()
}
} else {
TIMED_FUNCTION()
}
didn't seem as clean.
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 Well, no, but I quite like your example. Will give it a try to see how it would look like without looking at the code (mobile right now), my only concern would be using try-with-resource when we don't open a real resource nor throw any exception (can't recall that part). But I liked the start static method in special. WDYT @ottobackwards? ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user garydgregory commented on the issue:
https://github.com/apache/commons-lang/pull/311
Hi All:
Have you considered allowing the timer to be used in a try-with-resources
statement such that
```
StackWatch watch = new StackWatch<>("OuterFunction");
watch.start();
functionOne(watch);
watch.stop();
```
Becomes perhaps:
```
try (StackWatch watch = new
StackWatch<>("OuterFunction").start()) {
functionOne(watch);
}
```
or more succinctly:
```
try (StackWatch watch = StackWatch.start("OuterFunction")) {
functionOne(watch);
}
```
?
Gary
---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 Just a note, discussion of this issue has been split between here and the jira issue. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 @ottobackwards I'm +1 for using generics, as well as the fluent API suggested by Gilles just now. But I'd have to look at the code to see if that's doable. I think you are probably the best to take a look if that'd a) be a good idea, b) be doable, or c) argument why not. The generics I suspect might be easier to achieve. Fluent API's are normally useful, but easily to be overdone IMO. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 yes, i'm interested on your take on the jira stuff from today @kinow ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 Oh, and you have a pending comment in your JIRA ticket from another reviewer :-) ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 I would run something like `git rebase -i HEAD~123` where 123 is the number of commits I'm going back. Then squash the commits into a single one, discarding commit messages. Finally, would use a single commit message like "LANG-1373: The description here...". And then `git fetch --all` followed by a `git rebase origin/master` to make sure it's at the most recent change in the remote repository as well. If no merge conflicts, you can simply `push -f` to your branch, and that should do it. In case you are concerned about losing your changes, just do from your branch a `git checkout -b backup-1373` or some other name, `git checkout -` to go back to your branch, and give it a try (or do it in the other branch, whichever you prefer) ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 would a rebase -i with FIXUP work? What git command would *you* run? Do we have to worry about authorship? ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 E-mail sent. See the latest messages in the Commons Developers mailing list, with the tag [lang]. There should be one from me about this PR and your JIRA ticket, asking for more reviews. Thanks for being so patient @ottobackwards !! Bruno ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 Pull request looks good. Toyed with the code a bit (https://gist.github.com/kinow/c0873daf069c2e64bc69d6305400e610), and I like the current design. @ottobackwards let's ask in the ML if anybody else would like to review it, and if nobody replies by the weekend, consider it merged. During the merge, I'd like to squash the commits to keep the commit history/tree simpler. Would you mind squashing the commits and updating the pull request to one single commit (or a few if you think it's necessary), please? Cheers B ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 bump ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 Close and re-open to kick travis. My travis built fine, not sure why apache failed ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 I was the one who was not clear enough :-) sorry ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 I am so sorry, I'll take care of it. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 Sorry, what I meant was that indentation in the lang is done with 4 spaces. Added an example to another class. Hope that makes sense. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 I will take care of it :) ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 Perfect. So maybe it is not enforcing the 4 spaces? Haven't looked at the xml settings we have in lang in a while. So if you fixed the 2/4 spaces, we won't have any other nit-picks like this, and be able to focus on the feature. Especially since you added good docs, unit cases, and even a practical use case :-) ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 The reports actually look good for my stuff, except the coverage stuff, if I am reading this correctly ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 OK, I wasn't looking in target, I have the reports. smh. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 maybe i didn't run clean, i'll try again ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 Do I have to do something to get my stuff added to the reports in the local site? I see my tests run in the cli, but they are not in the reports, or the java doc etc ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 Thanks, I did try to follow that, I use travis so the different java builds worked. I used the local checkstyle xml, and through I had caught everything. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 Oh, good point. There's the [CONTRIBUTING.md](https://github.com/apache/commons-lang/blob/master/CONTRIBUTING.md). There are several links in that doc too, apologize for not having a shorter version. The tabs/spaces is mentioned at the first link under *Additional Resources*, but there are heaps of other useful information in there. Hopefully not too different from other ASF projects, so you can probably filter through 90% or more of the contents in those files, and just recognize one of two differences in the way [lang]/Commons expects patches and pull requests. Ta ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 You bet, thanks. If there is any guide that I should be following let me know. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 Ah, and might be a good idea to run `mvn clean site`, then have a look at the reports output. Some pull requests are delayed merging due to the introduction of findbugs/checkstyle/etc issues. Quickly running it and fixing any reported issues might make it easy and faster to get it merged :) ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user kinow commented on the issue: https://github.com/apache/commons-lang/pull/311 Ack :-) haven't had time to review it as we have a short summer around here, so have added a note to have a look at StopWatch (which I'm not familiar with) and at this variation. Said that, had a very brief peek at the code from the browser without using the IDE. The code looks great! Only small minor things I could see were a duplicated white line (which doesn't matter tbh) and the the formatting. If I recall correctly, [lang] uses 4 spaces instead of 2? Though I could be wrong. Thanks for being so patient. I intend to review it as soon as I get some spare time (IOW once the weather gets back to our normal 10-17C cloudy days with with windy rains). But happy if anyone beats me to it. Cheers Bruno ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 bump ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user ottobackwards commented on the issue: https://github.com/apache/commons-lang/pull/311 bump? ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/311 [](https://coveralls.io/builds/14967010) Coverage decreased (-0.009%) to 95.252% when pulling **25a0b403861c8be1937e1e06ec51ff2cfaf0 on ottobackwards:stackwatch** into **f5a9effebd7209f3fa5385f18a5e59e8a09122f2 on apache:master**. ---
[GitHub] commons-lang issue #311: LANG-1373 Stopwatch based capability for nested, na...
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/311 [](https://coveralls.io/builds/14886950) Coverage increased (+0.004%) to 95.264% when pulling **ddaab51568ab01fc883d30e66394a669a75e24cc on ottobackwards:stackwatch** into **f5a9effebd7209f3fa5385f18a5e59e8a09122f2 on apache:master**. ---
