Hi Minh Yes I agree, push this enhancement and then fix the #2757 bug. Have you noticed that the ticket number is wrong? shall be #2735! I think it is correct in the commit message
Thanks Lennart > -----Original Message----- > From: Minh Hon Chau [mailto:[email protected]] > Sent: den 11 januari 2018 23:33 > 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, > > When testing the patch of this ticket, I had did the test as you > suggested (just only switchover). The ReadNext API returns BAD_HANDLE > because the active NTFD does not update to the standby the info of > Reader Id, search criteria, etc.. I created ticket #2757. I found that > there is no any documentation (PR, README) or even some TODO(s) to note > that NTF does not checkpoint the Reader info. It prompts me a doubt that > this such behavior of the Reader was discussed before and agreed it was > ok for the Reader to work like this? But it looks to me as defect for > now and I'm in middle of fixing #2757 and try to get it ready with > #2375, otherwise we may have to step up mbcsv version. However, I would > like to finish and push #2375 first, since they are different issues > although both belongs to the NTF reader. > > Thanks, > > Minh > > > On 11/01/18 19:03, Lennart Lund wrote: > > 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
