Hi Alena, I updated the patch and included a fix for every suggestion. I can imagine that helping me with this took a lot of your time and you went through my patch thoroughly, so thank you very much.
If this update is accepted, I can send the next one soon, in which in addition to the changes I already did for the next patch I will bear in mind your comments to avoid the same ones ;-) Cheers Antonio 2014-02-27 18:55 GMT+01:00 Alena Prokharchyk <alena.prokharc...@citrix.com>: > > > On 2/26/14, 5:43 PM, "Antonio Fornié Casarrubios" > <antonio.for...@gmail.com> wrote: > > >Hi Alena, > > > > > >2014-02-26 23:19 GMT+01:00 Alena Prokharchyk > ><alena.prokharc...@citrix.com>: > > > >> Antonio, > >> > >> I still don't quite see why false result returned from one of the > >> workers, should silently stop other workers if those workers are a) > >> independent b) have their own way of processing the failures in > >> asynchronous manner later as you stated. > >> > > > >If you don't see why to stop the chain then you can just return true. On > >the other hand, the Chain of Responsibility Pattern from GoF always has > >been implemented in a way the each worker can invoke the next OR NOT. And > >again, this is very common in JEE filters, I'm sure you already found > >similar examples with JEE filters, right? > > > >Yes, workers are independent. And one worker can be designed to stop the > >chain under certain conditions even if this worker doesn't have a clue > >what > >workers come later. Then you decide to put this worker in the chain > >before, > >after or not to put it at all. Just like in CoR pattern from GoF. > > > >On the other hand, removing this flexibility has any benefit? You keep > >saying "silently", but not invoking a method that should not be executed > >is > >not something silent. That's because you still see false as "error" and > >not > >as "mission complete". > > > Where do you see this error, in the log? Only if admin looks at the log > and notices it, he would do something. And in this case "mission > incomplete" has 0 impact, IMHO. The validation of the request is > incomplete, yet you continue to proceed the request further. And it can > lead into all possible errors in the rest of the code. > > > Reading your email further down, I've came across: > > "And in the Dispatch Chain returning false > doesn't mean "I return false because I hide a RuntimeException", but more > "I receive a DispatchTask that I know is specifically for me, so I > consume/process it and stop the chain because I've been designed for that > purpose"." > > Its either has to be well documented, or the return statement has to be > changed to some enum value indicating "Stop the chain" action. > Because I believe most of the people would treat "false" returned by the > method as the indication of failure. > > > > > > >In general I prefer to choose non restricting choices. Just like I USUALLY > >prefer protected members over private ones, because in general I assume if > >another developer in the future decides to extend a method she will do it > >for reasons... even if I don't see the reasons in advance. I can also make > >the method private, but that only causes the developer will change the > >method to protected and then extend it when needed, speaking about this... > > > >... I really don't see is this conversation changing our points of view. I > >prefer to just return void, if anybody need to change this in the future > >they can always do it. > > > Ok, can you please change it to void then? It would be more clear. > > > > > > > > > >> You also say "didn't want each worker to have a reference to the > >>next.". > >> But in your case you do obey the reference in indirect when failure of > >>one > >> worker execution affects the other? I'm confused. > >> > > > >Didn't want each worker to have a reference to the next, but not because > >of > >decoupling, just because if WorkerA -> WorkerB then I cannot have a chain > >WorkerA -> AnotherWorker unless I create another instance of WorkerA > >(because a worker object only has one next). So my point was, better to > >create only one instance of each worker given that there will be not > >benefit on having each worker referencing the next. I never meant it was > >because of encapsulation... > > > >...because of encapsulation we do other things, but not that. We deal with > >workers by interface, but this is something you have whether you use list > >or wrappers, whether you return void or boolean. Having workers in a chain > >make it flexible and modular, but doesn't make imperative that the workers > >cannot affect each other or assume anything about others. Your assumptions > >in a worker should be documented so it's easy to know how to create the > >chain. something like: "This worker stops the chain if we decide to put > >the > >cmd in a queue due to X". You have to keep in mind how they affect each > >other in order to decide how to create the chains: that knowledge is the > >role of the factory. > > > >And actually the previous code (and many others) are like that: in a > >certain block of code sometimes you do something considering the code that > >may come later. And actually we do it in the workers: one of the workers > >(ParamUnpackWorker) changes the parameter format assuming that next > >workers > >will need the new format instead of the old one. Then it's up to you to > >keep workers in order: if you don't put this worker in the first place in > >the list then the other workers will fail because the params are not > >formatted. Thus this worker knows that the next workers will never get to > >the old format params... and that also happened before with the > >unpackParams method, although the method itself doesn't know all details > >about what will happen with the new format params later. > > > > > > > > > >> > >> As an example, we can look at couple of examples of something similar > >>in > >> CloudStack code. > >> > >> 1) SecurityChecker Adapters. All adapters are invoked on the resource, > >> and if something fails, it throws an exception that clearly stops others > >> checkers from execution. > >> > > > >In Spring Security you can configure several "agents" to decide to > >authorize access or not, and you can configure it to work if all the > >agents > >say YES or if AT LEAST ONE says yes. In the latter case, the moment one > >gives the YES it does stop the others from doing a check. You can find 20 > >examples in which you don't need to stop, but if you find a single example > >in which you do need it that should be enough. Why to remove the > >possibility just because you don't come across a good example? Why to keep > >it more restricted instead of more flexible? > > > > > > > >> 2) UserAuthenticator Adapter. When user logs in, all adapters defined > >>in > >> the config file, get invoked in specific order. If one of them fails > >> (RuntimeException is thrown), the login attempt fails as well, and other > >> adapters are not invoked. > >> > > > >But this is an example related with failure and I was never speaking about > >failure. Returning false was never a replacement from raising exceptions. > >ie: The method MyDao#countEntities returns a number and throws and > >exception if there are connection problems. Would you be afraid that > >someone will return -1 instead of throwing an exception? No, because the > >return is not used for that. And in the Dispatch Chain returning false > >doesn't mean "I return false because I hide a RuntimeException", but more > >"I receive a DispatchTask that I know is specifically for me, so I > >consume/process it and stop the chain because I've been designed for that > >purpose". > > > > > > > > > >> > >> In other words, if you decide that each worker returns boolean > >>variable, > >> it means that you have to process that result in the caller stack, which > >> the code-to-review doesn't do. You either: > >> > > > >:-) No! Above all the caller doesn't process anything because the details > >of the current chain may be transparent for the caller. The caller puts > >milk in the coffe machine and trusts the coffee will come, and he doesn't > >mind how many pipes the milk went through. Exceptions are different, and > >that would be the callers business. But the false/true is only for the > >iterator to decide if the work is done, not for the caller. And that's why > >the method DispatchChain#dispatch is void, although the internal method > >Worker#handle returns boolean. The caller doesn't mind the boolean, the > >iterator (DispatchChain) does. > > > > > > > >> * throw RuntimeException when one of the workers returns failure. It > >> would eliminate your fear that developer writing the worker, will > >>forget to > >> do it in handle() method itself > >> > > > >Exceptions are apart from the subject, because I never return false to > >avoid throwing an exception (actually I never return false at all but I > >design it to be open). > > > >* change the return type to void. I'm pretty sure the developers writing > >> the workers, will take care of throwing an exception on validation > >>failure > >> > > > >Of course they will do, whether we go left or right. I never thought a > >developer would return a false instead of throwing an exception just > >because he can. His patch would not pass the review :) And further he > >would > >read the javadoc header > > * @return false to stop the chain of responsibility, true > > * to continue the chain with the next worker > > > >...nothing mentions that false means error. > > > > > > > > > >> Whatever one you prefer from the above, I'm fine with. > >> > > > >Actually I'm fine with doing the changes you tell me in your next answer, > >but I cannot choose between these 2 options because the code never > >returned > >false in case of any error. > > > >I really want to have this patch through. I will do whatever changes you > >tell me in your next mail, and if you don't mind I will request your > >review > >for future related patches. Can you confirm what other things you finally > >want me to change in addition to this? > > > >Thank you very much again. > >Antonio > > > > > >> Let me know what you think, > >> -Alena. > >> > >> From: Antonio Fornié Casarrubios <antonio.for...@gmail.com> > >> Date: Wednesday, February 26, 2014 at 2:02 PM > >> To: Alena Prokharchyk <alena.prokharc...@citrix.com> > >> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, daan > >> Hoogland <daan.hoogl...@gmail.com>, Hugo Trippaers < > >> htrippa...@schubergphilis.com> > >> Subject: Re: Review Request 17888: Dispatcher corrections, refactoring > >> and tests. Corrects problems from previous attempt > >> > >> Hi Alena, > >> > >> Answer inline: > >> > >> > >> > >> 2014-02-26 22:24 GMT+01:00 Alena Prokharchyk > >><alena.prokharc...@citrix.com > >> >: > >> > >>> Antonio, please see my 2 comments inline. > >>> > >>> -Alena. > >>> > >>> On 2/26/14, 1:17 PM, "Antonio Fornié Casarrubios" > >>> <antonio.for...@gmail.com> wrote: > >>> > >>> >Hi Alena, > >>> > > >>> >I answer to your comments inline: > >>> > > >>> > > >>> >2014-02-26 19:08 GMT+01:00 Alena Prokharchyk > >>> ><alena.prokharc...@citrix.com>: > >>> > > >>> >> This is an automatically generated e-mail. To reply, visit: > >>> >> https://reviews.apache.org/r/17888/ > >>> >> > >>> >> Antonio, in general looks good to me. There are some minor fixes > >>>that > >>> >>need to be done though. > >>> >> > >>> >> 1) ApiServer.java > >>> >> > >>> >> @Inject() > >>> >> 177 > >>> >> protected DispatchChainFactory dispatchChainFactory = null; > >>> >> > >>> >> > >>> >> Why do you use @Inject with ()? > >>> >> > >>> >> > >>> >*I tried some parameters with the Inject annotation but finally I > >>> decided > >>> >to use it as it is here. Just a habit from using other types of DI > >>> >annotations. I just didn't didn't remove the () in this one, but as > >>>you > >>> >know it's exactly the same so it didn't even bring my attention. I can > >>> >change that or if you approved this patch, change it in the next > >>>patch. > >>> >Again, it's just the same.* > >>> > > >>> > > >>> > > >>> >> 2) ApiServlet.java > >>> >> > >>> >> domainIdArr = (String[])params.get(ApiConstants.DOMAIN__ID); > >>> >> > >>> >> * Why do we have DOMAIN__ID and DOMAIN_ID as separate parameters? > >>> >> > >>> >> public static final String DOMAIN_ID = "domainid"; > >>> >> public static final String DOMAIN__ID = "domainId"; > >>> >> > >>> >> The above doesn't look right to me. Why do we care about the case? > >>>the > >>> >>API parameter is always transformed to all lowercase letters > >>> >> > >>> >> *Having these almost identical Strings is not my change, ApiServlet > >>> was > >>> >already using both with I and i (domainid and domainId) so I just > >>>kept it > >>> >as it is (I assume removing one of them may cause a problem when it > >>>comes > >>> >in the req. In order to create the constants for both values I just > >>>chose > >>> >these names: no names would make much more sense because having this > >>>two > >>> >Strings were already something strange. There are several strange > >>>things > >>> >like this, I just fixed a few and moved plenty from hardcoded values > >>>to > >>> >constants, but no more than that, my main focus was something else and > >>> the > >>> >patch is already very big.* > >>> > > >>> >3) CommandCreationWorker.java > >>> >> > >>> >> > >>> >> throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, > >>> >> 50 > >>> >> "Trying to invoke creation on a Command that is > >>>not > >>> >> BaseAsyncCreateCmd"); > >>> >> > >>> >> > >>> >> * Don't hardcode the class name; retrieve it from the .simpleName > >>> >>method called on the class object > >>> >> > >>> >> *Retrieve it? This is a piece of code that was in > >>> >ApiDispatcher#dispatchCreateCmd just after calling processParameters, > >>>so > >>> >it > >>> >had to be a next worker in the chain, but I kept the code as it was. > >>>And > >>> >this specific method was only invoked after checking the command was > >>> >BaseAsyncCreateCmd (or any class extending, of course), so I created > >>>the > >>> >same check in the worker. And then of course I keep also the reason to > >>> >fail > >>> >in the error msg. If it fails the only thing I care is that it is not > >>>a > >>> >BaseAsyncCreateCmd as it should... so I don't understand what do you > >>> >propose there. For anybody who chages this code in the future, this > >>>error > >>> >will tell them they should not apply this worker when the dispatch > >>>chain > >>> >is > >>> >not the expected type.* > >>> > >>> > >>> :) What I¹ve meant is, instead of "Trying to invoke creation on a > >>>Command > >>> that is not BaseAsyncCreateCmd², log it as "Trying to invoke creation > >>>on > >>> a Command that is not ³ + BaseAsyncCreateCmd.class.getSimpleName(). So > >>>in > >>> case someone changes the class name in the future, you don¹t have to > >>>dig > >>> in into hardcoded stuff in order to change it, as it will change > >>> dynamically > >>> > >>> > >> *** I see, sorry I dind't understand you meant that. I will sure change > >> that, good point. > >> > >> > >> > >>> > >>> > > >>> >> 4) DispatchChain > >>> >> > >>> >> for (final DispatchWorker worker : workers) { > >>> >> if (!worker.handle(task)) { > >>> >> break; > >>> >> } > >>> >> } > >>> >> > >>> >> * Why do we just break when workder can't handle the task? Aren't we > >>> >>supposed to do something? > >>> >> * Can you please add more logging? At least log in trace that some > >>> >>worker handled/coudln't handle the task > >>> >> > >>> >> > >>> >> *There are many ways to create a Chain of Responsibility, for this > >>> case > >>> >>I > >>> >decided to go for a one way chain (instead of a typical two way flow) > >>>in > >>> >which workers share a task where they apply their work and pass their > >>> >changes. They also have the power to stop the chain if the programmer > >>> >decides so in her code. I'm not assuming the worker can't handle > >>>anything > >>> >or not not doing something, I am just making sure I provide a way to > >>>stop > >>> >the chain if one of the workers decides so. If there were something to > >>> >log, > >>> >the worker would know what and if something requires so I guess they > >>>will > >>> >raise and exception instead of just returning false. We don't have > >>> >anything > >>> >to log just because the behavior was to stop the chain. Just see it as > >>> JEE > >>> >filters where they can decide to do whatever even there is no error.* > >>> > >>> > >>> > >>> Stopping and doing nothing after all doesn¹t look right to me, as you > >>> don¹t pass the result to the caller stack to process it further > >>>depending > >>> on the return type. I would rather change work.handle() return type to > >>> void. As as you said, worker would throw an exception anyway if > >>>something > >>> goes wrong. > >>> > >>> > >> *** There is no "Stopping and doing nothing", you create your chains > >> considering the order of workers and you code the workers considering > >>what > >> you want to do in the chain. You should be free to consider the work > >>done. > >> Just like you decide to execute some code or not with an if, and that > >> doesn't mean you are doing nothing: there is nothing to do if the clause > >> for the if is not true: this is the same. ie: Shouldn't a worker be > >>able to > >> decide to save the cmd for later execution asynchronously and when > >> retrieved from a queue go through another chain? I don't know if this > >>is a > >> good example, but actually for something similar we are using ifs and > >> instanceof checks to choose the flow, right? Again the JEE filters are > >>the > >> best example and they are also used to process/dispatch a request/cmd. > >> > >> The reference implementation of Chain of Responsibility is based on > >> workers that wrap the next one recursively (like JEE filters) and thus > >>each > >> worker is free to invoke the next worker or not. With your proposal we > >> loose that. Actually I prefer to go for the that reference > >>implementation, > >> but I decided to go for a list with boolean return types because: 1) We > >> don't need pre and post process, it's a one way chain (at least so far) > >>and > >> also because 2) I want to create the workers as Spring singletons > >>(default > >> scope), but I also want to be able to have several chains with different > >> workers so I didn't want each worker to have a reference to the next. > >> Someone could could brake it by returning false when you shouldn't just > >> like you could by throwing an exception when you shouldn't, but that's > >>not > >> a reason not to give this flexibility to the workers. I think it's > >>better > >> to assume that new workers will be coded appropriately. > >> > >> By the way and in case this info helps (I will include it also in the > >> FS), some more clarification: The old way in the ApiDispatch was this > >>way: > >> > >> hugeMethod(){ > >> // Some code for doing A > >> > >> // Some code for doing B > >> > >> if (cmd instanceof CmdTypeX) { > >> // Some code for doing C > >> } > >> > >> if (N) { > >> } else { > >> // some code for doing D > >> } > >> } > >> > >> > >> And the new approach is: > >> Chain for X > >> A > B > C > >> Chain for N > >> A > B > D > >> > >> So in the end what you could decide to do based on ifs or anything else, > >> you can decide now based on the chain workers and on stopping the chain > >>or > >> not. > >> > >> For the next changes I plan to do more refactoring in this direction. I > >> also did another kind of change so that instead of doing instanceof > >>checks > >> so often (we have them everywhere in CS code) we use polymorphism. For > >>this > >> I created the SemanticValidationWorker so that there the work is > >>actually > >> delegated to the Cmd class (polymorphism) > >> > >> > >> > > >>> > > >>> >> > >>> >> 4) I can see that you do a lot of initialization like that for > >>>private > >>> >>variables: > >>> >> > >>> >> protected AccountManager _accountMgr = null; > >>> >> > >>> >> its not necessary step as private variables are being initialized to > >>> >>NULL by default during declaration. Can you please clean it out? > >>> >> > >>> >> > >>> >> > >>> >*I know. In Java local variables are not initialized, but instance > >>>ones > >>> >are > >>> >(booleans to false, numbers to cero, objects to null...). But I find > >>>it > >>> >more clear to put it this way, further people may ignore or forget it, > >>> >specially non Java people helping in cloudstack. It's also something > >>>I've > >>> >seen in many Java projects. Again, it's just the same. If you think > >>>this > >>> >is > >>> >not clean and should be changed before the patch is accepted, I can > >>> change > >>> >that too.* > >>> > > >>> >*Upon reception of your comments I will change and upload anything.* > >>> > > >>> >- Alena Prokharchyk > >>> >> > >>> >> On February 26th, 2014, 9:12 a.m. UTC, Antonio Fornie wrote: > >>> >> Review request for cloudstack, Alena Prokharchyk, daan Hoogland, > >>>and > >>> >> Hugo Trippaers. > >>> >> By Antonio Fornie. > >>> >> > >>> >> *Updated Feb. 26, 2014, 9:12 a.m.* > >>> >> *Repository: * cloudstack-git > >>> >> Description > >>> >> > >>> >> Dispatcher corrections, refactoring and tests. Corrects problems > >>>from > >>> >>previous attempts that were reverted by Alena. Most of the changes > >>>are > >>> >>the same, but this one is not creating conflicts of Map types for > >>>Aync > >>> >>Commands or for parameters as Lists or Maps. > >>> >> > >>> >> Testing > >>> >> > >>> >> Full build and test plus manually testing many features. Also > >>>including > >>> >>CreateTagsCommand that failed in previous commit. > >>> >> > >>> >> All unit and integration tests. > >>> >> > >>> >> Test CS Web UI with the browser going through several use cases. > >>> >> > >>> >> Also use the CS API by sending HTTP requests generated manually > >>> >>including requests for Async Commands with Map parameters and during > >>> >>these tests apart fromtesting correct functionality I also debugged > >>>to > >>> >>check that Maps created correctly where they should but also that in > >>>the > >>> >>cases where the async command must be persisted and later on > >>>retrieved > >>> >>and deserialized by gson everything works ok and does what and where > >>>is > >>> >>expected. An example based on the comment by > >>> >>Alena: > >>> http://localhost:8096/client/api?command=createTags&resourceids=ids > >>> >>&resourcetype=type&tags[0].key=region&tags[0].value=canada > >>> >> Also other examples > >>> >>like > >>> http://localhost:8096/client/api?command=createSecondaryStagingStore&; > >>> > >>> > >>>>>url=httpbla&details[0].key=region&details[0].value=canada&details[1].k > >>>>>ey= > >>> >>element&details[1].value=fire > >>> >> > >>> >> Diffs > >>> >> > >>> >> - api/src/org/apache/cloudstack/api/ApiConstants.java (7b7f9ca) > >>> >> - api/src/org/apache/cloudstack/api/BaseCmd.java (0e83cee) > >>> >> - api/src/org/apache/cloudstack/api/BaseListCmd.java (c1a4b4c) > >>> >> - > >>> >>api/src/org/apache/cloudstack/api/command/admin/user/GetUserCmd.java > >>> >> (592b828) > >>> >> - > >>> > >>>>>api/src/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.jav > >>>>>a > >>> >> (de6e550) > >>> >> - > >>> > >>> > >>>>>api/src/org/apache/cloudstack/api/command/user/autoscale/CreateAutoSca > >>>>>leV > >>> >>mProfileCmd.java > >>> >> (06d86ec) > >>> >> - > >>> > >>> > >>>>>server/resources/META-INF/cloudstack/core/spring-server-core-misc-cont > >>>>>ext > >>> >>.xml > >>> >> (fd2f5fb) > >>> >> - server/src/com/cloud/api/ApiAsyncJobDispatcher.java (71ac616) > >>> >> - server/src/com/cloud/api/ApiDispatcher.java (ed95c72) > >>> >> - server/src/com/cloud/api/ApiServer.java (d715db6) > >>> >> - server/src/com/cloud/api/ApiServlet.java (46f7eba) > >>> >> - server/src/com/cloud/api/dispatch/CommandCreationWorker.java > >>> >> (PRE-CREATION) > >>> >> - server/src/com/cloud/api/dispatch/DispatchChain.java > >>> (PRE-CREATION) > >>> >> - server/src/com/cloud/api/dispatch/DispatchChainFactory.java > >>> >> (PRE-CREATION) > >>> >> - server/src/com/cloud/api/dispatch/DispatchTask.java > >>>(PRE-CREATION) > >>> >> - server/src/com/cloud/api/dispatch/DispatchWorker.java > >>> >>(PRE-CREATION) > >>> >> - > >>> server/src/com/cloud/api/dispatch/ParamGenericValidationWorker.java > >>> >> (PRE-CREATION) > >>> >> - server/src/com/cloud/api/dispatch/ParamProcessWorker.java > >>> >> (PRE-CREATION) > >>> >> - > >>> >>server/src/com/cloud/api/dispatch/ParamSemanticValidationWorker.java > >>> >> (PRE-CREATION) > >>> >> - server/src/com/cloud/api/dispatch/ParamUnpackWorker.java > >>> >> (PRE-CREATION) > >>> >> - server/src/com/cloud/network/as/AutoScaleManagerImpl.java > >>> (ff2b2ea) > >>> >> - > >>>server/src/com/cloud/storage/snapshot/SnapshotSchedulerImpl.java > >>> >> (3159059) > >>> >> - server/test/com/cloud/api/ApiDispatcherTest.java (7314a57) > >>> >> - > >>>server/test/com/cloud/api/dispatch/CommandCreationWorkerTest.java > >>> >> (PRE-CREATION) > >>> >> - > >>>server/test/com/cloud/api/dispatch/DispatchChainFactoryTest.java > >>> >> (PRE-CREATION) > >>> >> - > >>> > >>>>>server/test/com/cloud/api/dispatch/ParamGenericValidationWorkerTest.ja > >>>>>va > >>> >> (PRE-CREATION) > >>> >> - server/test/com/cloud/api/dispatch/ParamProcessWorkerTest.java > >>> >> (PRE-CREATION) > >>> >> - > >>> > >>> > >>>>>server/test/com/cloud/api/dispatch/ParamSemanticValidationWorkerTest.j > >>>>>ava > >>> >> (PRE-CREATION) > >>> >> > >>> >> View Diff <https://reviews.apache.org/r/17888/diff/> > >>> >> > >>> > >>> > >> > >
