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/> >>> >> >>> >>> >>
