Hi Anders Widell, I incorporated most of your comments except NCS_PATRICIA_TREE refactoring to std::map. Please find my response with [AVM] & code changes V2 patch (lgs: director Cluster Membership (CLM) integration [#1638] V2)
-AVM On 7/27/2016 6:54 PM, Anders Widell wrote: > Hi! > > Some comments: > > 1) Have you considered using std::map instead of adding a new instance > of NCS_PATRICIA_TREE? > 2) At least when adding completely new files (lgs_clm.cc in this > case), could you try to keep them "clean" from cppcheck and cpplint > warnings (i.e. make sure there are no warnings in newly added files)? [AVM] I will do all NCS_PATRICIA_TREE conversion to std::map of Log service in one in a separate ticket soon. > 3) You have added the new file lgs_clm.cc, but there is no > corresponding lgs_clm.h header file. Could you also add the file > lgs_clm.h and put the public API of lgs_clm.cc in it? [AVM] Done. > 4) lgs_clm.cc has only one single #include statement, which includes > "lgs.h". But it uses many interfaces that are not declared in lgs.h. > You should not rely on header files being indirectly included from > other header files. lgs_clm.cc ought to include all headers that it > needs, for example: "saClm.h" for the CLM API, <cstdint> for uint32_t > et.al., "logtrace.h" for the OpenSAF trace API, <cstdlib> for the > exit() function and the EXIT_FAILURE constant, <pthread.h> for the > pthreads API. [AVM] Done. > 5) Isn't the variable name usr2_sel_obj misleading? There is another > variable with the very similar name usr1_sel_obj, and it is a > selection object which is used when the process receives the SIGUSR1 > signal. I would have expected that usr2_sel_obj would correspondingly > be used when the process receives the SIGUSR2 signal. However, it > seems that instead it is actually used to signal an MDS up event for > AMFD. I think it ought to be renamed to reflect this. Or maybe even > get rid of this selection object and instead post a message in the > mailbox when we get this MDS event? [AVM] Replaced `usr2_sel_obj` with `clm_init_sel_obj` > 6) Why is the usr2_sel_obj created only in the case the when the LOG > service is started by NID? This only makes sense for the usr1_sel_obj > which is used to signal AMF instantiation when the process has already > been started by NID. To me it looks like the code currently doesn't > work when it is not started by NID - in this case clmSelectionObject > will be initialized to -1 and stay that way forever. [AVM] Removed the nid_started dependency. > 7) I suppose the whole reason for triggering CLM initalization after > MDS up event from AMFD, using this usr2_sel_obj variable, is to solve > some circular dependency problem. But is it at all needed when CLM > initialization is taken care of in a separate thread? Wouldn't it work > fine if you simply start this CLM initialization thread when you > receive an AMF assignment (i.e. start the thread from inside the > initialize_for_assignment() function)? Could you try it? :-) [AVM] LGS will/can have agents prior to AMF , so spawning CLM initialization thread upon AMF assignment may unnecessarily delay the CLM member status to those agent. > 8) In C++, please use nullptr instead of NULL and 0 (when the type is > supposed to be a pointer). So e.g. clm_callbacks should have have > nullptr instead if 0 in its first position, and lgs_clm_init_thread > ought to return nullptr instead of NULL. - AVM > > thanks, > Anders Widell > > On 07/26/2016 10:42 AM, [email protected] wrote: >> Summary:lgsv: Log Service CLM integration [#1638] V1 >> Review request for Trac Ticket(s): #1638 >> Peer Reviewer(s): Vu, Lennart >> Pull request to: <<LIST THE PERSON WITH PUSH ACCESS HERE>> >> Affected branch(es): Default >> Development branch: Default >> >> -------------------------------- >> Impacted area Impact y/n >> -------------------------------- >> Docs n >> Build system n >> RPM/packaging n >> Configuration files n >> Startup scripts n >> SAF services y >> OpenSAF services n >> Core libraries n >> Samples n >> Tests n >> Other n >> >> >> Comments (indicate scope for each "y" above): >> --------------------------------------------- >> >> changeset 0637f0bfc39b5173776b3cdca088cffd28425432 >> Author: A V Mahesh <[email protected]> >> Date: Tue, 26 Jul 2016 14:07:35 +0530 >> >> lga: agent Cluster Membership (CLM) integration [#1638] V1 >> Description: >> >> Form CLM integration is supported from Log Service A.02.02. >> >> At-least a A.02.02 LGA client will check CLM membership status of >> client's >> node. old LGA clients A.02.01 are always clm member. >> >> This patch enhanced the log service for Unavailability of the Log >> Service >> API on a Non-Member Node which will fail with >> SA_AIS_ERR_UNAVAILABLE. >> >> After this patch the Log Service does not provide service to >> processes on >> cluster nodes that are not in the cluster membership. >> >> If the node rejoins the cluster membership, processes executing >> on the node >> will be able to reinitialize new library handles and use the >> entire set of >> Log Service APIs that operate on these new handles; however, >> invocation of >> APIs that operate on handles acquired by any process before the >> node left >> the membership will continue to fail with SA_AIS_ERR_UNAVAILABLE >> (or with >> the special treatment described above for asynchronous calls) >> with the >> exception of saLogFinalize(), which is used to free the library >> handles and >> all resources associated with these handles. Hence, it is >> recommended for >> the processes to finalize the library handles as soon as the >> processes >> detect that the node left the membership. >> >> Detailed README will be provide soon. >> >> Following are expected Log Service API behavior : >> >> Case1: On Non-Member Node, Log Service API will fail with code >> SA_AIS_ERR_UNAVAILABLE (31) Case2: On Member Node after recovered >> from Non- >> Member Node, Log Service API will fail with code >> SA_AIS_ERR_UNAVAILABLE (31) >> Case3: Non-Member Node + (Headless) Log Service API will fail >> with code >> SA_AIS_ERR_UNAVAILABLE (31) Case4: On Non-Member Node + >> (Headless) + (Head >> Joined) Log Service API will fail with code >> SA_AIS_ERR_UNAVAILABLE (31) >> >> changeset fed6d0af1a288640e078bdac58b00c0c79691e97 >> Author: A V Mahesh <[email protected]> >> Date: Tue, 26 Jul 2016 14:08:09 +0530 >> >> lgs: director Cluster Membership (CLM) integration [#1638] V1 >> >> changeset 6842daa277070d2ac2c80eb6c69271521a51d563 >> Author: A V Mahesh <[email protected]> >> Date: Tue, 26 Jul 2016 14:09:35 +0530 >> >> logtest: change related to Cluster Membership (CLM) integration >> [#1638] V1 >> Test : >> >> #amf-adm lock safNode=PL-4,safCluster=myClmCluster >> >> <saLogStreamOpen_2> failed with code SA_AIS_ERR_UNAVAILABLE (31). >> >> #amf-adm unlock safNode=PL-4,safCluster=myClmCluster >> >> <saLogStreamOpen_2> failed with code SA_AIS_ERR_UNAVAILABLE (31). >> >> #amf-adm lock safNode=PL-4,safCluster=myClmCluster + (SC-1 & Sc-2 >> /etc/init.d/opensafd stop) >> >> <saLogStreamOpen_2> failed with code SA_AIS_ERR_UNAVAILABLE (31) >> >> # amf-adm lock safNode=PL-4,safCluster=myClmCluster + ((SC-1 & Sc-2 >> /etc/init.d/opensafd stop)) + ((SC-1 & Sc-2 /etc/init.d/opensafd >> start) + amf-adm unlock safNode=PL-4,safCluster=myClmCluster >> >> <saLogStreamOpen_2> failed with code SA_AIS_ERR_UNAVAILABLE (31) >> >> Expected but currently The Imm APIs are NOT working error - >> saImmOmInitialize FAILED: SA_AIS_ERR_LIBRARY (2) >> >> >> Complete diffstat: >> ------------------ >> osaf/libs/agents/saf/lga/lga.h | 2 + >> osaf/libs/agents/saf/lga/lga_api.c | 37 >> +++++++++++++++++++++++++++++++++++ >> osaf/libs/agents/saf/lga/lga_mds.c | 75 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> osaf/libs/agents/saf/lga/lga_util.c | 2 + >> osaf/libs/common/logsv/include/lgsv_defs.h | 6 ++++- >> osaf/libs/common/logsv/include/lgsv_msg.h | 17 ++++++++++++--- >> osaf/services/saf/logsv/lgs/Makefile.am | 4 ++- >> osaf/services/saf/logsv/lgs/lgs.h | 5 ++++ >> osaf/services/saf/logsv/lgs/lgs_cb.h | 15 ++++++++++++++ >> osaf/services/saf/logsv/lgs/lgs_clm.cc | 143 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> osaf/services/saf/logsv/lgs/lgs_evt.cc | 142 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> osaf/services/saf/logsv/lgs/lgs_evt.h | 1 + >> osaf/services/saf/logsv/lgs/lgs_main.cc | 29 >> +++++++++++++++++++++++++++ >> osaf/services/saf/logsv/lgs/lgs_mds.cc | 41 >> +++++++++++++++++++++++++++++++++++++- >> osaf/services/saf/logsv/lgs/lgs_util.cc | 83 >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> tests/logsv/saflogtest.c | 2 +- >> 16 files changed, 595 insertions(+), 9 deletions(-) >> >> >> Testing Commands: >> ----------------- >> <<LIST THE COMMAND LINE TOOLS/STEPS TO TEST YOUR CHANGES>> >> >> >> Testing, Expected Results: >> -------------------------- >> <<PASTE COMMAND OUTPUTS / TEST RESULTS>> >> >> >> Conditions of Submission: >> ------------------------- >> <<HOW MANY DAYS BEFORE PUSHING, CONSENSUS ETC>> >> >> >> Arch Built Started Linux distro >> ------------------------------------------- >> mips n n >> mips64 n n >> x86 n n >> x86_64 y y >> powerpc n n >> powerpc64 n n >> >> >> Reviewer Checklist: >> ------------------- >> [Submitters: make sure that your review doesn't trigger any checkmarks!] >> >> >> Your checkin has not passed review because (see checked entries): >> >> ___ Your RR template is generally incomplete; it has too many blank >> entries >> that need proper data filled in. >> >> ___ You have failed to nominate the proper persons for review and push. >> >> ___ Your patches do not have proper short+long header >> >> ___ You have grammar/spelling in your header that is unacceptable. >> >> ___ You have exceeded a sensible line length in your >> headers/comments/text. >> >> ___ You have failed to put in a proper Trac Ticket # into your commits. >> >> ___ You have incorrectly put/left internal data in your comments/files >> (i.e. internal bug tracking tool IDs, product names etc) >> >> ___ You have not given any evidence of testing beyond basic build tests. >> Demonstrate some level of runtime or other sanity testing. >> >> ___ You have ^M present in some of your files. These have to be removed. >> >> ___ You have needlessly changed whitespace or added whitespace crimes >> like trailing spaces, or spaces before tabs. >> >> ___ You have mixed real technical changes with whitespace and other >> cosmetic code cleanup changes. These have to be separate commits. >> >> ___ You need to refactor your submission into logical chunks; there is >> too much content into a single commit. >> >> ___ You have extraneous garbage in your review (merge commits etc) >> >> ___ You have giant attachments which should never have been sent; >> Instead you should place your content in a public tree to be >> pulled. >> >> ___ You have too many commits attached to an e-mail; resend as threaded >> commits, or place in a public tree for a pull. >> >> ___ You have resent this content multiple times without a clear >> indication >> of what has changed between each re-send. >> >> ___ You have failed to adequately and individually address all of the >> comments and change requests that were proposed in the initial >> review. >> >> ___ You have a misconfigured ~/.hgrc file (i.e. username, email etc) >> >> ___ Your computer have a badly configured date and time; confusing the >> the threaded patch review. >> >> ___ Your changes affect IPC mechanism, and you don't present any results >> for in-service upgradability test. >> >> ___ Your changes affect user manual and documentation, your patch series >> do not contain the patch that updates the Doxygen manual. >> >> >> ------------------------------------------------------------------------------ >> >> >> What NetFlow Analyzer can do for you? Monitors network bandwidth and >> traffic >> patterns at an interface-level. Reveals which users, apps, and >> protocols are >> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >> J-Flow, sFlow and other flows. Make informed decisions using capacity >> planning >> reports.http://sdm.link/zohodev2dev >> _______________________________________________ >> Opensaf-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/opensaf-devel >> > ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
