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.
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.
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.
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
