I just want to make my point clear to everyone. 1. Sid's idea is right and ideal. 2. We are suffering from the trunk build failing due with a *one liner wrong if statement* (let me know if you think I'm wrong on this) 3. The test code by using asynchronous has been *running this way for some time* and designed by some one else. (and I didn't know anything about it) 4. Thus, I just want to fix the one line (if statement) and get the trunk build work. 5. We make another ticket to Sid's point and will come back.
Don't get me wrong. I agree Sid's idea is the right thing, but it is more like refactoring. I'm only interested in getting trunk work by fixing the wrong if statement. - jun On Fri, Oct 10, 2014 at 10:44 AM, John Speidel <[email protected]> wrote: > I don't feel that it would be appropriate to merge without addressing > Sid's concerns. > > -John > > On Fri, Oct 10, 2014 at 1:34 PM, Alejandro Fernandez < > [email protected]> wrote: > >> >> >> > On Oct. 9, 2014, 10:28 p.m., Sid Wagle wrote: >> > > >> ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java, >> line 431 >> > > < >> https://reviews.apache.org/r/26510/diff/2/?file=717035#file717035line431> >> > > >> > > The NPE seems to be from, >> > > line 782: commandParamsStageCache.put(stagePk, commandParams) >> > > >> > > Seems like we have command params as null so guava cache >> complains about it. Maybe, >> "StageUtils.getGson().fromJson(s.getCommandParamsStage(), type)" returns >> null for empty string, note: we are passing empty string for >> comandParamsStage. >> > > >> > > Not sure how this would be resolved after some time? >> > > A null check in ActionScheduler might not be a bad idea, your >> call. >> > >> > jun aoki wrote: >> > Sid, yes it is from StageUtils.getGso()... call returning a null. >> > I'm not too sure either how it is resolved but it has been working >> this way and we solved asychronous behaviour. I'd like to say it is good to >> go. Please give a Ship it if you are OK. >> > >> > Sid Wagle wrote: >> > Could you try putting a Null check after, line 782: >> commandParamsStageCache.put(stagePk, commandParams) and removing the while >> condition? >> > If that works there is no need to catch the NPE, I am not convinced >> NPE is expected behavior. >> >> I concur. >> >> >> - Alejandro >> >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/26510/#review56070 >> ----------------------------------------------------------- >> >> >> On Oct. 9, 2014, 10:05 p.m., jun aoki wrote: >> > >> > ----------------------------------------------------------- >> > This is an automatically generated e-mail. To reply, visit: >> > https://reviews.apache.org/r/26510/ >> > ----------------------------------------------------------- >> > >> > (Updated Oct. 9, 2014, 10:05 p.m.) >> > >> > >> > Review request for Ambari and Yusaku Sako. >> > >> > >> > Bugs: AMBARI-7622 >> > https://issues.apache.org/jira/browse/AMBARI-7622 >> > >> > >> > Repository: ambari >> > >> > >> > Description >> > ------- >> > >> > Tweaked the waiting condition upon ActionScheduler >> > >> > >> > Diffs >> > ----- >> > >> > >> >> ambari-server/src/test/java/org/apache/ambari/server/actionmanager/TestActionScheduler.java >> a20f252 >> > >> > Diff: https://reviews.apache.org/r/26510/diff/ >> > >> > >> > Testing >> > ------- >> > >> > >> > Thanks, >> > >> > jun aoki >> > >> > >> >> > > CONFIDENTIALITY NOTICE > NOTICE: This message is intended for the use of the individual or entity > to which it is addressed and may contain information that is confidential, > privileged and exempt from disclosure under applicable law. If the reader > of this message is not the intended recipient, you are hereby notified that > any printing, copying, dissemination, distribution, disclosure or > forwarding of this communication is strictly prohibited. If you have > received this communication in error, please contact the sender immediately > and delete it from your system. Thank You. > -- -jun
