[GitHub] [geode] boglesby opened pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["boglesby (GitHub)" <gi...@apache.org>]

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.


[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["mcmellawatt (GitHub)" <gi...@apache.org>]

Functionally this might not be a big deal, but there is a new ConcurrencyRule that offers helper methods to add several runnables or callables, then execute them in parallel.  It might save you a few lines of code here and it uses an ExecutorService under the hood.  You should be able to search for ConcurrencyRule and see usages in our code.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["mcmellawatt (GitHub)" <gi...@apache.org>]

Could be made `final`

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

I see the old code also did this. Perhaps it used to have some other stuff around r.run that has since been removed.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["kirklund (GitHub)" <gi...@apache.org>]

Honestly, I think using final on local variables really just adds a lot of noise. People used to claim that it sped up the code, but I've never seen that proven, plus the JIT figures things out so fast that any improvement would be very short-lived.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

I think this code might be easier to understand if you get rid of this first if (isBufferConsumer || isFunctionExecutionThread) and just had "if (isBufferConsumer) ... else if (isFunctionExecThread) ... else".
The first two can both have calls to launchAdditionalThread.
I think it would help to split up the large comment into three; each in one block of this code.
It might even be worth extraction these three blocks into their own methods (then your comment can be on those methods) 


[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

feel free not to make this change, but...
you could combine line 98 and 99 into:
  } else if {
and get rid of an extra layer of indentation.


[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] dschneider-pivotal commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["dschneider-pivotal (GitHub)" <gi...@apache.org>]

seems like this could just be ".newThread(r);" What is the point of creating a runnable that just calls r.run?

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] boglesby commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["boglesby (GitHub)" <gi...@apache.org>]

Thanks. I had just refactored a new method. You're right. I don't see any reason to create a new Runnable there. I'll change that.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] boglesby closed pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["boglesby (GitHub)" <gi...@apache.org>]

[ pull request closed by boglesby ]

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] boglesby commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["boglesby (GitHub)" <gi...@apache.org>]

Thanks. I'll change this as you suggest.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] boglesby commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["boglesby (GitHub)" <gi...@apache.org>]

That was an attempt to get all the parent functions executing before the child functions were invoked. Its definitely hacky and not actually necessary. I took it out.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] mcmellawatt commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["mcmellawatt (GitHub)" <gi...@apache.org>]

Just curious what this Thread.sleep() is used for?  Might justify a comment.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] kirklund commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["kirklund (GitHub)" <gi...@apache.org>]

Another option is the ExecutorServiceRule in geode-junit. If you're firing off threads one at a time and don't need to line them up at a start line to start them all at once then ExecutorServiceRule is probably the better option.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] boglesby commented on pull request #2750: GEODE-5959: Modified to launch a thread in the nested function case

["boglesby (GitHub)" <gi...@apache.org>]

Thanks! I'll look into that rule.

[ Full content available at: https://github.com/apache/geode/pull/2750 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org