Hi Canh, I forgot to attach some comments I made in the code
Thanks Lennart From: Lennart Lund Sent: den 2 augusti 2018 14:32 To: Canh Van Truong <canh.v.tru...@dektech.com.au>; Vu Minh Nguyen <vu.m.ngu...@dektech.com.au> Cc: opensaf-devel@lists.sourceforge.net; Lennart Lund <lennart.l...@ericsson.com> Subject: RE: [PATCH 0/1] Review Request for log: fix logtest 6 42 fail [#2903] Hi Canh Thanks for the information. See my comments below Regards Lennart From: Canh Van Truong <canh.v.tru...@dektech.com.au<mailto:canh.v.tru...@dektech.com.au>> Sent: den 2 augusti 2018 07:12 To: Lennart Lund <lennart.l...@ericsson.com<mailto:lennart.l...@ericsson.com>>; Vu Minh Nguyen <vu.m.ngu...@dektech.com.au<mailto:vu.m.ngu...@dektech.com.au>> Cc: opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: RE: [PATCH 0/1] Review Request for log: fix logtest 6 42 fail [#2903] Hi Lennart Please see my reply inline Thanks Canh -----Original Message----- From: Lennart Lund <lennart.l...@ericsson.com<mailto:lennart.l...@ericsson.com>> Sent: Wednesday, August 1, 2018 9:20 PM To: Canh Van Truong <canh.v.tru...@dektech.com.au<mailto:canh.v.tru...@dektech.com.au>>; Vu Minh Nguyen <vu.m.ngu...@dektech.com.au<mailto:vu.m.ngu...@dektech.com.au>> Cc: opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net>; Canh Van Truong <canh.v.tru...@dektech.com.au<mailto:canh.v.tru...@dektech.com.au>>; Lennart Lund <lennart.l...@ericsson.com<mailto:lennart.l...@ericsson.com>> Subject: RE: [PATCH 0/1] Review Request for log: fix logtest 6 42 fail [#2903] Hi Canh, I am not sure this is a correct fix. Please answer my comments [Lennart]: Test case logtest 2 49 creates some cfg app to test the limitation, then deleting these stream at the end. But the deleting fails due to timeout for waiting the implementer. After that test case logtest 6 42 need to create some cfg app and take the same stream name. That causes the creattion fail with SA_AIS_ERR_EXIST error. [Lennart] This part will make it possible to run the second test case (6 42) if the first test case (2 49) fails to delete the log streams it creates. Before the fix the streams was given the same name in both test cases. However if you should try to run test (2 49) again it will fail for the same reason as test case (6 42) failed so the problem is actually not solved. [Canh] I think the case you mention here is that you MANUALLY try to run test case (2 49) 2 times separately. To avoid it, you have to delete existed object manually before running second test case (2 49). The fix just solves when you run the whole test cases "logtest", each test case executed sequentially and we cannot involve to delete existed object. [Lennart] Testing of the log service may be done on several nodes in the same test session. If for example logtest is run on node1 and this problem occurs the will continue. However, if logtest is done again on another node2 test 2 49 will fail as above since in an automated test nobody will MANUALLY remove anything. The purpose of both test cases is that test the limitation of number of app stream and it doesn't verify if deleting stream fail or not at the end of test cases. In deep, lib function rename() return failure causes the retry is happened. From the second retry, it cannot find the old file due to file name is changed. Actually the file name is changed (check file name in directory "saflog/..."). This rarely happen (first time I see it). Maybe no way to fix this. How do you think about it? [Lennart] I'm a little bit confused here, something with this seems wrong. Test 2 49 and 6 46 is supposed to test the limitation of the number of APPLICATION streams (app streams) that shall be possible to create in a cluster and was introduced with ticket "#1446 log: trouble when the number of existing app streams reaches limitation" The header information for test case 2 49 says (and something similar for 6 42): "/* * Add test case for ticket #1446 * Verify that logsv failed to create runtime app stream if number of app * streams has reached the limitation * */" However, test 2 49 and 6 42 creates a number of CONFIGURATION streams and tests that no more than the number of streams set in IMM attribute logMaxApplicationStreams can be created. This is incorrect, there is no limitation for CONFIGURATION streams. The limitation set in logMaxApplicationStreams is only for APPLICATION streams! When I look at the test cases there seems to be some confusion about the definitions of application stream and configuration stream and there properties. An APPLICATION stream is a stream that is always (and can only be) created in runtime using the log service API saLogStreamOpen_2(). The stream is created if in-parameter *logFileCreateAttributes is not set to NULL, shall be set to NULL if an existing stream shall be opened. An app stream is represented by a runtime object in the IMM model. An app stream is always automatically deleted when closed by all clients. There is a limitation for how many app streams that can exist in a cluster, the limitation is set in the log service configuration object attribute logMaxApplicationStreams. A CONFIGURATION stream can be defined in the configuration of the IMM model and is represented by a configuration object. Three such streams always exist (specified in AIS) which are the alarm stream, the notification stream and the system stream. User defined configuration streams can also be defined. A user defined configuration stream can be defined in the IMM model configuration file(s) so that it exists at installation time. It is also possible to create a configuration stream in runtime by creating a configuration object of class SaLogStreamConfig. A configuration stream can be opened by a client in the same way as an already existing application stream by using the log service API saLogStreamOpen_2() but cannot be created using that API. A configuration stream is not deleted when closed by all clients. A configuration stream is always created automatically when the cluster is started. It is possible to delete a user defined configuration stream by removing the corresponding configuration object and that can be done in runtime. Creation and deletion of configuration streams in runtime is done using IMM APIs for create or delete of configuration object. NOTE1: There shall be no limitation of the number of configuration streams! If such a limitation exist it is DEFECT that should be fixed! NOTE2: It is not logical that the log service shall check how many configuration streams there are in a cluster since the log service is not used to define them. The TESTS: A test that verifies that it is not possible to create more APPLICATION streams than given by the limit in logMaxApplicationStreams is valid to have but a test for validating a max number of CONFIGURATION streams is not. - If a limitation of number of CONFIGURATION streams that may exist in a cluster is implemented in the log server it is a defect and a ticket should be written and fixed. The log service may in practice only be able to handle a limited number of log streams but in that case this should be a documented limitation. If I remember correctly the code may be written in such a way that the memory allocated for log stream data (for all streams) is based on the value in logMaxApplicationStreams + 3 or something like that but that is internal bad code that could be fixed. - The test cases should be changed to verify max number of APPLICATION streams. The patch updates stream name of 2 test case and update the retry of renaming file is 5s due to imm wait for implemeter 6s. [Lennart] Why is this changed? This is not a change in the test code. Originally timeout for renaming files was 8 seconds and you have changed that to 5 seconds. I cannot see the meaning of that. I assume that the log service deletes the log stream using immcfg (only for cfg streams) after this timeout even if the file could not be renamed and that the thinking behind this is that log stream deletion shall be done before timeout in immcfg. However, the immcfg tool is using immutil for handling try again loops and the timeout setting (see immutilWrapperProfile) is 60 seconds which is the recommended timeout time for initializing an in this case OM handle (also for OI handle). Note that changing the file renaming timeout may be considered NBC! [Canh] - Timeout setting (immutilWrapperProfile) is 60 second that isn't meaningful here. The timeout I mention is that timeout, imm wait for callback from implementer (I got from Vu is 6 seconds). So I changed timeout here to 5s. Note that imm does not care how long ccb_app_callback execute. But if ccb_app_callback take a lot of time, it will impact to next ccb wait for implementer response (e.g ccb_completed_callback of next ccb doesn't execute because lgs hasn't completed the previous ccb_app_callback). * From original code, it didn't use time clock to calculate the timeout (used variable "msecs_waited"). So actually the time to retry here is bigger 8s. I update to use time clock instead variable. * I am not sure why NBC? [Lennart] Now I understand that the test is not testing APPLICATION streams and then the callback handling timeout makes some sense. However, Stream_close() is not used only when deleting CONFIGURATION streams. Shortening the timeout may be an NBC if the log service is running on a system with a file system that at least sometimes is a bit slow but the 8 sec timeout is enough but a new 6 sec timeout is not. This means that in such a system the log service starts to fail more often than before. This is NBC. Thanks Lennart > -----Original Message----- > From: Canh Van Truong > <canh.v.tru...@dektech.com.au<mailto:canh.v.tru...@dektech.com.au>> > Sent: den 1 augusti 2018 05:15 > To: Lennart Lund > <lennart.l...@ericsson.com<mailto:lennart.l...@ericsson.com>>; Vu Minh Nguyen > <vu.m.ngu...@dektech.com.au<mailto:vu.m.ngu...@dektech.com.au>> > Cc: > opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net>; > Canh Van Truong > <canh.v.tru...@dektech.com.au<mailto:canh.v.tru...@dektech.com.au>> > Subject: [PATCH 0/1] Review Request for log: fix logtest 6 42 fail [#2903] > > Summary: log: fix logtest 6 42 fail [#2903] > Review request for Ticket(s): 2903 > Peer Reviewer(s): Vu, Lennart > Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE *** > Affected branch(es): develop,release > Development branch: ticket-2903 > Base revision: 7f6f6c0531a0f5e4f2b0dc1abf4bab6962a3d1a9 > Personal repository: git://git.code.sf.net/u/canht32/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 > > > Comments (indicate scope for each "y" above): > --------------------------------------------- > *** EXPLAIN/COMMENT THE PATCH SERIES HERE *** > > revision 7489c44113ac179020ba554b0c76eaf9750ef75f > Author: Canh Van Truong > <canh.v.tru...@dektech.com.au<mailto:canh.v.tru...@dektech.com.au>> > Date: Wed, 1 Aug 2018 10:02:21 +0700 > > log: fix logtest 6 42 fail [#2903] > > Test case logtest 2 49 creates some cfg app to test the limitation, then > deleting these stream at the end. But the deleting fails due to timeout for > waiting the implementer. After that test case logtest 6 42 need to create > some cfg app and take the same stream name. That causes the creattion fail > with SA_AIS_ERR_EXIST error. > > The patch updates stream name of 2 test case and update the retry of > renaming > file is 5s due to imm wait for implemeter 6s. > > > > Complete diffstat: > ------------------ > src/log/apitest/tet_LogOiOps.c | 20 ++++++++++---------- > src/log/apitest/tet_saLogStreamOpen_2.c | 10 +++++----- > src/log/logd/lgs_stream.cc | 23 ++++++++++------------- > 3 files changed, 25 insertions(+), 28 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 n n > 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.
log_2903_Lennart_coments.diff
Description: log_2903_Lennart_coments.diff
------------------------------------------------------------------------------ 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 Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
