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.
