JCLOUDS-624 - Fixed bug in ListNodes #47

luciano-sabenca-movile · 2014-07-14T17:23:23Z

Details can be found at: https://issues.apache.org/jira/browse/JCLOUDS-624

Any suggestions are very welcome!

To fix this bug, I used the approach debated in the above link: create
new methods to do the operation using an ExecutorService provided by the
user.The the old methods are still working, but now the operations in
these methods are not concurrent anymore!

buildhive · 2014-07-14T17:28:09Z

jclouds » jclouds-chef #1171 SUCCESS
This pull request looks good
(what's this?)

cloudbees-pull-request-builder · 2014-07-14T17:29:15Z

jclouds-chef-pull-requests #109 SUCCESS
This pull request looks good

nacx · 2014-07-14T23:04:10Z

Thanks @luciano-sabenca-movile! A couple comments:

I'd modify the ChefService interface and the strategies to have only a method that accepts a regular ExecutorService, and internally call the listeningDecorator. It is safe, as it will do nothing if the passed object is already a ListeningExecutorService and will simplify the interfaces considerably.
Is it really needed to split the classes and create the base class? The logic is not that long. Couldn't the methods in the base class be part of the class itself? As I see it, strategies represent one concrete behavior, and it is unlikely to have subclasses of the "base" class. Subclassing just the strategy class could be a more common approach if someone needs to customize them.

luciano-sabenca-movile · 2014-07-15T14:56:41Z

Hi, @nacx !

So, just to explain my thoughts:

Yes, in fact, when the method is called using an {code} ExecutorService {code} internally this executor is decorated using a {code} listeningDecorator {code}. I will remove these methods.
In my opinion, the code is better divided in a base class. I don't like the ideia of duplicate code, even if the code is not so long. Although strategies represents a concrete behavior, it is represented using interfaces, so the base class is completely hided by these interfaces, to customize the behavior it is just necessary extend the strategy implementation class or just implement the interface. So, for me, the "base" class is just a way to avoid duplicated code, it doesn't disturbs the logic and the concepts involved on it.

Regards

nacx · 2014-07-15T15:23:18Z

What I see is that the logic in the base class is very tied to the logic of the concrete strategy itself. It has the execute and executeConcurrently methods, but they're not generic. They perform very concrete operations by calling the concrete api.get* methods, so they would be mostly reused by the strategy itself. That's why I think it makes sense to have them in the same class.

The concrete strategy class only performs the "list" operation, and I can't think about a different strategy that could take benefit from using only the base class, therefore my feeling that this is kind of a premature optimization that is just adding unnecessary complexity given the existing implementations of the strategies.

Anyway, there's no need to change it now :) Let's change the unnecessary methods from the interface and let's see if someone else has something to say.

buildhive · 2014-07-15T17:31:31Z

jclouds » jclouds-chef #1174 SUCCESS
This pull request looks good
(what's this?)

cloudbees-pull-request-builder · 2014-07-15T17:32:30Z

jclouds-chef-pull-requests #110 SUCCESS
This pull request looks good

luciano-sabenca-movile · 2014-07-15T17:32:36Z

Ok, @nacx!

No problem :-) ! I really enjoy this this kind of debate, they are is very insightful!

I've committed the changes in the interface! If we merge it in the main repository, I think that is a good idea squash these commits in a single one, but this is a point for the future.

nacx · 2014-07-15T21:50:19Z

I've committed the changes in the interface!

I meant to remove all methods that accept a ListeningExecutorService from the ChefService and from the strategies. Then in the strategy implementations, always call the listeningDecorator, as it won't do nothing if the parameter is already a ListeningExecutorService.

It does not make sense to have an interface with a generic executor and a concrete one. It is like having an interface that has two methods, one accepting a List and one accepting an ArrayList.

luciano-sabenca-movile · 2014-07-17T19:53:10Z

Yes, you are right, @nacx !

It was my fault!, I'm sorry about it! It makes no sense keep those methods in the interface.
I've just fix it! I removed all methods from the interface that receives a ListeningExecutorService!

Thanks!

cloudbees-pull-request-builder · 2014-07-17T19:54:24Z

jclouds-chef-pull-requests #111 SUCCESS
This pull request looks good

buildhive · 2014-07-17T19:55:54Z

jclouds » jclouds-chef #1185 SUCCESS
This pull request looks good
(what's this?)

nacx · 2014-07-18T10:05:19Z

Thanks for the cleanup @luciano-sabenca-movile!

I've checked out the code and now I understand those base classes. They are reused in the strategies that list all entities, and in strategies that list only the entities in one environment. My concerns were because I thought there were only used by one class, thus my comments about the premature optimization. Other strategies don't have the logic in an abstract base class, so it is OK.

There are a couple things left before merging the PR:

The ListEnvironment strategy still declares a ListeningExecutorService in the interface. Could you change it to a standard ExecutorService?
Run a mvn checkstyle:checkstyle and fix the checkstyle violations. You'll find them in the target/checkstyle-result.xml output file.

The PR looks good, and once these small bits are fixed I'll merge it. Thanks!

buildhive · 2014-07-18T14:42:23Z

jclouds » jclouds-chef #1187 SUCCESS
This pull request looks good
(what's this?)

luciano-sabenca-movile · 2014-07-18T14:42:43Z

Oh, the remaining methods in ListEnvironment were a little absence of mind :-)!

In fact, the reason to create these abstracts base class is to apply the same operations but in distinct sets.

cloudbees-pull-request-builder · 2014-07-18T14:43:09Z

jclouds-chef-pull-requests #112 SUCCESS
This pull request looks good

nacx · 2014-07-18T14:47:28Z

Thanks @luciano-sabenca-movile! Changes lgtm.
Before merging, could you squash the commits into a single one?

luciano-sabenca-movile · 2014-07-18T14:48:38Z

Of course! I will do this right now!

luciano-sabenca-movile · 2014-07-18T14:52:32Z

Ok.

Done!
Now, all the changes are in just one commit!

demobox · 2014-07-18T14:53:17Z

core/src/main/java/org/jclouds/chef/ChefService.java

+    */
+   Iterable<? extends Node> listNodesConcurrently(ExecutorService executorService);
+
+


[minor] Remove blank line?

cloudbees-pull-request-builder · 2014-07-18T14:56:49Z

jclouds-chef-pull-requests #113 SUCCESS
This pull request looks good

demobox · 2014-07-18T14:57:08Z

core/src/main/java/org/jclouds/chef/ChefService.java

+   @SinceApiVersion("0.10.0")
+   Iterable<? extends Node> listNodesInEnvironmentConcurrently(
+         ExecutorService executorService, String environmentName
+   );


See comment above.

buildhive · 2014-07-18T14:57:23Z

jclouds » jclouds-chef #1188 SUCCESS
This pull request looks good
(what's this?)

demobox · 2014-07-18T14:57:30Z

core/src/main/java/org/jclouds/chef/ChefService.java

+    */
+   Iterable<? extends Client> listClientsConcurrently(ExecutorService executorService);
+
+


[minor] Remove blank line

demobox · 2014-07-18T15:02:10Z

@luciano-sabenca-movile Thanks for submitting this! I don't want to disrupt this PR at this late stage, since we can always follow up with another one. Just three main comments:

adding concurrent actions to jclouds here seems to go rather against the direction we're talking elsewhere, where async stuff is being removed and the recommendation is for users to do the async in client code
if we feel parallelism is essential as a speedup here, why are we even keeping the old methods?
if we do want to allow both approaches, an Options pattern would seem to be more consistent with the rest of jclouds, and also allows for future enhancements such as paging etc.

I like the fact we're allowing the user to provide the executor, by the way!

@nacx Thoughts..?

demobox · 2014-07-18T15:03:19Z

PS: I'm guessing this is the new version of #46, by the way..?

nacx · 2014-07-18T15:22:28Z

I.e. we add them now and immediately deprecate them? If that is the case, is there not another solution we should be looking for?

Note that the methods in the strategy interfaces that accepted an executor were already there. it's not an addition but a generalization.

PS: Also fine to continue this discussion after this is merged, if this is an urgent bug fix...

It's not urgent :) I'd like to have this in before moving Chef to the main site, but I prefer to have an agreement first on how we want things to look like.

nacx · 2014-07-18T15:42:28Z

After the recent discussion, I think we could add some final changes to leave things consistent and easier to use for end users:

Previous versions of jclouds used the internal jclouds executor and fetched the nodes concurrently by default.
With this change nodes will be by default obtained in sequence, but users may want to keep the old behavior and keep getting them in parallel.

So given that client changes will be required for those users that still want to get benefit of concurrency, I think it is OK to apply the options change in the interfaces, at least to remove the "async" stuff from them. I'd suggesto to:

Rename all methods prefixed with concurrently to overload the normal methods.
~~Create a ListOptions object, as suggested by @demobox that has a concurrently option that accepts an ExecutorService.~~
~~Refactor the stategy interfaces too to accept that options object, and implement them accordingly.~~

Existing methods will still work, but without concurrency, but getting things done concurrently would require client changes anyway, so I think it is worth the change and it will leave the interfaces in a cleaner status (and allow us to easily improve them when needed). @demobox @luciano-sabenca-movile WDYT?

demobox · 2014-07-18T15:53:51Z

WDYT?

The suggestion looks good to me (unsurprisingly ;-)), but if we feel it's too much work at this late stage for something that won't be around for that long anyway, I'd also be OK with dropping the Options object and simply having an ExecutorService argument at the end of the overloaded method.

At the very least, we could decouple the two steps and do the simple method renaming (i.e. dropping "xxxConcurrently") first, then considering the Options refactoring in a separate PR.

nacx · 2014-07-21T08:22:29Z

I've thought about this a bit more this weekend, and I think it should be better to just address the renaming of the *Concurrently methods now. We're about to release 1.8.0, and it would be ideal if jclouds-chef could be merged into the main repo for the release.

With the renaming change, the feature will be completed and without backwards incompatible changes, so it looks like a good plan just to complete the PR with the renaming, then move jclouds-chef to the main repo, and consider the ListOptions thing after the release. Are you OK with this @demobox @luciano-sabenca-movile?

luciano-sabenca-movile · 2014-07-21T19:30:34Z

@demobox, @nacx, very nice discussion :-)!

WDYT?

I agree with the solution! It seems very good to me!

About the simpler solution to release 1.8.0, I think that is a good way to address it. It is important release version 1.8.0 with this bug fixed, because this one invalidates important methods(ListNodes for example), and - adopting this solution - we can implement a complete refactor later with tranquility.

nacx · 2014-07-21T19:52:58Z

Let's do this then. Rename the methods to remove the "Concurrently" prefix and address the options thing after 1.8.0 in the main jclouds repo.

buildhive · 2014-07-21T21:16:58Z

jclouds » jclouds-chef #1198 FAILURE
Looks like there's a problem with this pull request
(what's this?)

cloudbees-pull-request-builder · 2014-07-21T21:16:59Z

jclouds-chef-pull-requests #114 FAILURE
Looks like there's a problem with this pull request

To fix this bug, I used the approach debated in the above issue: create new methods to do the operation using an ExecutorService provided by the user.The the old methods are still working, but now the operations in those methods are not concurrent anymore.

buildhive · 2014-07-21T21:35:22Z

jclouds » jclouds-chef #1199 SUCCESS
This pull request looks good
(what's this?)

luciano-sabenca-movile · 2014-07-21T21:35:55Z

Done!

@nacx, @demobox, please take a look at this! :-D

cloudbees-pull-request-builder · 2014-07-21T21:36:23Z

jclouds-chef-pull-requests #115 SUCCESS
This pull request looks good

nacx · 2014-07-24T08:41:22Z

Thanks @luciano-sabenca-movile! Changes lgtm. @demobox goot to merge?

nacx · 2014-07-28T13:32:38Z

Commited to master. Thanks!

demobox · 2014-07-28T14:15:53Z

core/src/main/java/org/jclouds/chef/ChefService.java

@@ -168,20 +175,44 @@
   Iterable<? extends Node> listNodesInEnvironment(String environmentName);

   /**
+    * Lists the details of all existing nodes in the given environment, using the ExecutorService to paralleling the execution.


[minor] "to parallelize"

demobox · 2014-07-28T14:39:04Z

Thanks, @luciano-sabenca-movile and @nacx! Just some minor follow-up comments.

Thanks for waiting for me - it's taken a bit longer to get back to this than I had hoped... ;-)

nacx · 2014-07-28T15:13:50Z

Thanks for waiting for me

In fact it is already merged :) But will address the comments post 1.8, as all are minors.

demobox · 2014-07-28T15:24:05Z

In fact it is already merged

I know...I meant: "Thank you for giving me so much time before merging" ;-)

demobox reviewed Jul 18, 2014
View reviewed changes

nacx closed this Jul 28, 2014

demobox reviewed Jul 28, 2014
View reviewed changes

		*/
		Iterable<? extends Node> listNodesConcurrently(ExecutorService executorService);

		*/
		Iterable<? extends Client> listClientsConcurrently(ExecutorService executorService);

JCLOUDS-624 - Fixed bug in ListNodes #47

JCLOUDS-624 - Fixed bug in ListNodes #47

Conversation

luciano-sabenca-movile commented Jul 14, 2014

buildhive commented Jul 14, 2014

cloudbees-pull-request-builder commented Jul 14, 2014

nacx commented Jul 14, 2014

luciano-sabenca-movile commented Jul 15, 2014

nacx commented Jul 15, 2014

buildhive commented Jul 15, 2014

cloudbees-pull-request-builder commented Jul 15, 2014

luciano-sabenca-movile commented Jul 15, 2014

nacx commented Jul 15, 2014

luciano-sabenca-movile commented Jul 17, 2014

cloudbees-pull-request-builder commented Jul 17, 2014

buildhive commented Jul 17, 2014

nacx commented Jul 18, 2014

buildhive commented Jul 18, 2014

luciano-sabenca-movile commented Jul 18, 2014

cloudbees-pull-request-builder commented Jul 18, 2014

nacx commented Jul 18, 2014

luciano-sabenca-movile commented Jul 18, 2014

luciano-sabenca-movile commented Jul 18, 2014

demobox Jul 18, 2014

Choose a reason for hiding this comment

cloudbees-pull-request-builder commented Jul 18, 2014

demobox Jul 18, 2014

Choose a reason for hiding this comment

buildhive commented Jul 18, 2014

demobox Jul 18, 2014

Choose a reason for hiding this comment

demobox commented Jul 18, 2014

demobox commented Jul 18, 2014

nacx commented Jul 18, 2014

nacx commented Jul 18, 2014

demobox commented Jul 18, 2014

nacx commented Jul 21, 2014

luciano-sabenca-movile commented Jul 21, 2014

nacx commented Jul 21, 2014

buildhive commented Jul 21, 2014

cloudbees-pull-request-builder commented Jul 21, 2014

buildhive commented Jul 21, 2014

luciano-sabenca-movile commented Jul 21, 2014

cloudbees-pull-request-builder commented Jul 21, 2014

nacx commented Jul 24, 2014

nacx commented Jul 28, 2014

demobox Jul 28, 2014

Choose a reason for hiding this comment

demobox commented Jul 28, 2014

nacx commented Jul 28, 2014

demobox commented Jul 28, 2014