Hi Minh Thanks for the answers.
Ack. See some new comments below [Lennart] Also, You have added a test where the standby node is restarted before switch over meaning that the cached notifications will be lost (testing cold sync). However also a switch over without restart should be tested. Maybe the same test case can be used for both tests. First run the test case without restart and then again with a restart. Just change the requested sequence. What do you think? Also backwards compatibility of the new mbcsv version should be tested (maybe you have done this already) before pushing. No new testcase is needed for this. See also inline comment. Thanks Lennart > -----Original Message----- > From: Minh Hon Chau [mailto:[email protected]] > Sent: den 11 januari 2018 00:31 > To: Lennart Lund <[email protected]>; > [email protected]; Canh Van Truong > <[email protected]> > Cc: [email protected] > Subject: Re: [PATCH 0/2] Review Request for ntf: Support cold sync for > cached alarms [#2375] > > Hi Lennart, > > Thanks for comments, please see my replies in line. > > Thanks, > > Minh > > > On 10/01/18 23:48, Lennart Lund wrote: > > Hi Minh, > > > > Some comments: > > > > TEST: > > Test does not give correct info for manual testing and as far as I can see > there is no information about how to run it in a test environment like > Osaftest. > > 1. > > Incorrect an not needed information saying that rda_ge_role failed. > Payload node. Run CLM tests. > > This information should not be printed at all since it gives no information > that is needed for handling the test case. Also it is incorrect, seems to be > some cut and paste from the CLM test suite. > [Minh]: No, the rda_get_role is called as part of constructor function > of CLM test. It has been there for a while, we can correct it in another > ticket to address the CLM test suite. [Lennart] OK > > 2. > > Information about node start/stop and switch over is incorrect. After > standby node is stopped it must be started again before a switch over can be > done > > > > Also there is no information about what is actually tested. This information > is for example needed when documentation about what's tested in > OpenSAF is created > [Minh]: I will add more comments/description to the test, and correct > the start/stop indication > > > > DESIGN: > > When a new feature is implemented it is always based on some sort of > design. This design should be described. If the design concerns several files > it > can be described in a README file. The description should tell what the new > feature is, the general idea that is implemented, what is added and changed > and maybe what files that are affected etc. > > In this case; Is anything changed in the current handling and why? What is > added and why and how is it working in general terms e.g. is there a new > protocol for cold sync? What does the new protocol looks like etc. > [Minh]: There is not noticeable thing to add in regard to the > implementation as well as the design, it only adds to cached alarms to > cold sync which have been missed. One thing we can document in README is > that the cold sync of cached alarms requires mbcsv of verison 3 onwards, > will update the README in next review. [Lennart] Ok. I assume mbcsv version 3 is created because of this new feature so please document what differs this version from version 2. Another thing, is backwards compatibility with mbcsv version 2 tested? What I mean is running version 3 on the active node but version 2 on standby and vice versa. > > > > IMPLEMENTATION: > > New functions are added where it is not obvious what they are doing, > when they are used, if they are dependent on anything etc. > > Some functions do have a function header but the header contains the > wrong information, not enough information or there is no header. > > This means that the code is not easy to review, will be hard (costly) to > maintain and it is not easy to get things right (see example for possible > bug). > > > > We should not introduce any new functions that are not correctly > documented and if old functions are used and they are not correctly > documented they have to be analyzed and base on that documented. This > activity will help to prevent possible mistakes as in the example below. It > will > also make the code easier to maintain. > > > > An example: > > When writing this I found that notifications maybe now are logged twice? > In that case a bug is introduced > > > > /** > > * A cached notification is received in Cold Sync > > * > > * @param clientId Node-wide unique id for the client who sent the > notifiaction. > > * @param notificationType > > * Type of the notification (alarm, object change etc.). > > * @param sendNotInfo > > * Pointer to the struct that holds information about the > > * notification. > > */ > > void NtfAdmin::cachedNotificationReceivedColdSync(... > > > > This header says that this function receives a cached notification in Col > Sync. This is not what this function is doing. Also the name of the function > is > misleading > > It is impossible to know what this function is doing without analyzing the > code and not only the code in this function. > > What it is actually doing is that it creates a notification object on the > > heap > and the pointer to this object is a shared (old type that should no longer be > used) pointer. > > This pointer is given to a function, "logger.log(notification, false)" > > which is > logging the notification using the log service but that is not what we want to > do here since I assume this " cachedNotificationReceivedColdSync()" > function is called on the standby node? If that's the case the notification > will > be logged twice. The "logger.log()" function also adds the given pointer to > some list of notifications that I assume is used with the reader API and that > is > probably what we want to do. > [Minh]: Will add more comments/description to headers. It won't be > logged twice, the notification is marked "logged" before add to the > list. Also, the logger.log(notif, false), false here means this notif > won't be queued and logged by LOG service, this notif only is added to > the cached list, which is currently hold by the Logger class. [Lennart] Ok. I assume this "flag" handling of what the function shall do is old legacy code. However in general this is not a good design. It is better if a function/method have only one purpose. If both storing and logging is needed then a store function should be used for storing and a log function for logging. > > Some guidance for how to document code can be found in the Google style > guide > > > > Thanks > > Lennart > > > >> -----Original Message----- > >> From: Minh Chau [mailto:[email protected]] > >> Sent: den 4 januari 2018 07:36 > >> To: Lennart Lund <[email protected]>; > >> [email protected]; Canh Van Truong > >> <[email protected]> > >> Cc: [email protected]; Minh Hon Chau > >> <[email protected]> > >> Subject: [PATCH 0/2] Review Request for ntf: Support cold sync for > cached > >> alarms [#2375] > >> > >> Summary: ntfd: Add cached alarms for cold sync [#2735] > >> Review request for Ticket(s): 2735 > >> Peer Reviewer(s): Lennart, Srinivas, Canh > >> Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE *** > >> Affected branch(es): develop > >> Development branch: ticket-2735 > >> Base revision: 60f929be8d7414257ecf90461f923b6f40ed3ac7 > >> Personal repository: git://git.code.sf.net/u/minh-chau/review > >> > >> -------------------------------- > >> 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 > >> > >> NOTE: Patch(es) contain lines longer than 80 characers > >> > >> Comments (indicate scope for each "y" above): > >> --------------------------------------------- > >> *** EXPLAIN/COMMENT THE PATCH SERIES HERE *** > >> > >> revision 725139c5df3cc0e208679f18a73ef874c5302755 > >> Author: Minh Chau <[email protected]> > >> Date: Thu, 4 Jan 2018 17:28:25 +1100 > >> > >> ntftest: Add new test case for cold sync of cached alarms [#2735] > >> > >> Patch add new test case of new suite 41 to test > saNtfNotificationReadNext > >> after the cold sync of cached alarm > >> > >> > >> > >> revision 2e45ed8d798857c0613954a77f7b7169ca74f3bf > >> Author: Minh Chau <[email protected]> > >> Date: Wed, 3 Jan 2018 15:35:19 +1100 > >> > >> ntfd: Add cached alarms for cold sync [#2735] > >> > >> Patch adds the alarm and security alarm notifications to cold > >> sync towards the standby NTFD, increase mbcsv version to avoid > >> confusion with old NTFD > >> > >> > >> > >> Added Files: > >> ------------ > >> src/ntf/apitest/tet_coldsync.c > >> > >> > >> Complete diffstat: > >> ------------------ > >> src/ntf/Makefile.am | 3 +- > >> src/ntf/apitest/tet_coldsync.c | 211 > ++++++++++++++++++++++ > >> src/ntf/apitest/tet_ntf.h | 2 + > >> src/ntf/apitest/tet_ntf_common.c | 97 +++++++++- > >> src/ntf/apitest/tet_ntf_common.h | 2 + > >> src/ntf/apitest/tet_ntf_main.c | 3 + > >> src/ntf/apitest/tet_scOutage_reinitializeHandle.c | 77 +------- > >> src/ntf/ntfd/NtfAdmin.cc | 33 +++- > >> src/ntf/ntfd/NtfAdmin.h | 3 + > >> src/ntf/ntfd/NtfLogger.cc | 16 ++ > >> src/ntf/ntfd/NtfLogger.h | 1 + > >> src/ntf/ntfd/NtfNotification.cc | 15 ++ > >> src/ntf/ntfd/NtfNotification.h | 1 + > >> src/ntf/ntfd/ntfs_com.c | 11 ++ > >> src/ntf/ntfd/ntfs_com.h | 5 + > >> src/ntf/ntfd/ntfs_mbcsv.c | 45 ++++- > >> src/ntf/ntfd/ntfs_mbcsv.h | 3 +- > >> 17 files changed, 446 insertions(+), 82 deletions(-) > >> > >> > >> Testing Commands: > >> ----------------- > >> Run legacy ntftest and new test of suite 41 > >> > >> > >> Testing, Expected Results: > >> -------------------------- > >> All pass > >> > >> > >> Conditions of Submission: > >> ------------------------- > >> ack from reviewers > >> > >> > >> 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 ~/.gitconfig file (i.e. user.name, user.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. > > ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
