Hi Nguyen, I have added what you suggested in the new review patch almost straight off. I also found a few issues myself in SmfExecControlHdl and SmfAdminState. I have removed some class variables and functions that are no longer used. This is actually a leftover from increment 1 but I have fixed it here. I will not send out any new review request. I will do some more testing on this latest variant and if there are no more comments and the test pass I will push this tomorrow
Thanks Lennart From: Nguyen Luu [mailto:nguyen.tk....@dektech.com.au] Sent: den 15 maj 2018 14:12 To: Lennart Lund <lennart.l...@ericsson.com>; Hans Nordebäck <hans.nordeb...@ericsson.com> Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [devel] [PATCH 1/1] smf: Add capability to redo CCBs that fail [#1398] Hi Lennart, I've looked at your changes in the .diff file. They look good overall. - Nevertheless, I think some updates, mostly regarding comments and log/trace printouts, are needed to make the changes consistent. You can find the places to update in the attached .diff file which can be applied onto your review repository. - Regarding renaming legacy function names in SmfImmOperation.h, you missed to change two legacy functions of SmfImmDeleteOperation class. Please see the attached .diff file for details and double-check my suggested update. - About refactoring SMF TRY_AGAIN handling for IMM operations, I agree that it should be done in a new ticket. (I have removed the original patch from the mail thread to reduce the message size) Thanks, Nguyen On 5/14/2018 8:13 PM, Lennart Lund wrote: Hi Nguyen, Attached patch contains all fixes. Can be applied on the review request. I will not send out a new review request unless Hans will find something that requires significant changes. This is how I have handled your comments: SmfAdminState.cc I have added most of your comments. Thanks for going through all comments and finding the mismatches between comments and variable names etc. that was wrong. A few of the suggested changes are not added. SmfCampState.* I have not changed the order of #include as suggested. The reason is that it cannot easily be done since there are problems with .h files that does not have all needed includes and circular dependencies. I tried to fix this in some parts of the code but run into huge problems so I skipped that in the legacy code. SmfCampaignWrapup.cc Your comment ... // [Nguyen] smfCreateRollbackElement eventually calls immutil_saImmOiRtObjectCreate_2() // which already handles TRY_AGAIN. Should we remove the TRY_AGAIN handling here? // I've also looked at the smfCreateRollbackElement part of other campaign stages and // found no use of such doImmOpTimer. ... is correct regarding the try again loop. This "outer loop" adds a timed loop with a timeout of 60 sec which is completely useless since the setting for immutil set by smf is to always use 600 X 1000 ms which is not at all correct. However I will not change any of this since it is not within scope of this ticket. Maybe create a new ticket for this? SmfImmOperation.h: // [Nguyen] The naming of class methods in this file is not consistent, // i.e some with capitalized initial letter, some with uncapitalized initial letter // Should we capitalize all initial letter for consistency? // getClassName() -> GetClassName() This again is legacy code. I have not renamed all legacy functions according to Google style guide so there may be a mix in some classes. However this file is fixed. SmfUtils.cc: lldtest_delete is test code that should have been removed. Fixed Thanks Lennart -----Original Message----- From: Nguyen Luu [mailto:nguyen.tk....@dektech.com.au] Sent: den 10 maj 2018 09:46 To: Lennart Lund <lennart.l...@ericsson.com><mailto:lennart.l...@ericsson.com>; Hans Nordebäck <hans.nordeb...@ericsson.com><mailto:hans.nordeb...@ericsson.com> Cc: opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: Re: [devel] [PATCH 1/1] smf: Add capability to redo CCBs that fail [#1398] Hi Lennart, I've reviewed your patch. Code review only, not tested yet. Please see my comments in the attached diff file which was generated on top of the changes in your patch. Regards, Nguyen ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
