Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/19640 )
Change subject: [Python] Support setting runtime flags in tests ...................................................................... Patch Set 5: (5 comments) http://gerrit.cloudera.org:8080/#/c/19640/1/python/kudu/tests/common.py File python/kudu/tests/common.py: http://gerrit.cloudera.org:8080/#/c/19640/1/python/kudu/tests/common.py@133 PS1, Line 133: > Is it possible to introduce a constant with human-friendly name for this (e Done http://gerrit.cloudera.org:8080/#/c/19640/1/python/kudu/tests/common.py@149 PS1, Line 149: > ditto Done http://gerrit.cloudera.org:8080/#/c/19640/3/python/kudu/tests/common.py File python/kudu/tests/common.py: http://gerrit.cloudera.org:8080/#/c/19640/3/python/kudu/tests/common.py@131 PS3, Line 131: > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/19640/3/python/kudu/tests/test_common.py File python/kudu/tests/test_common.py: http://gerrit.cloudera.org:8080/#/c/19640/3/python/kudu/tests/test_common.py@22 PS3, Line 22: class TestKuduTestBaseRuntimeFlags(KuduTestBase, CompatUnitTest): > nit in this file: add space after a comma when separating arguments for a f Done http://gerrit.cloudera.org:8080/#/c/19640/3/python/kudu/tests/test_common.py@47 PS3, Line 47: with self.assertRaisesRegex(Exception, error_msg): > Could you add a scenario to check how it fares on an attempt to set a value In the above example 'check_expired_table_interval_seconds' is not a runtime flag, (https://github.com/martongreber/kudu/blob/master/src/kudu/master/master.cc#L104) however the set function call goes true. IIUC SetDaemonFlagRequestPB is handled in https://github.com/apache/kudu/blob/master/src/kudu/mini-cluster/external_mini_cluster.h#L545 And here it states that it uses the 'force' flag. So no matter what, the flag gets set. Obviously the effect depends on the type of the flag. A little bit of context: I'm working on adding the soft-delete functionality to the Python client. To check that tables are deleted after a shorter period of time, I have to change 'check_expired_table_interval_seconds' to a shorter time from the default 60[s]. There I noticed that by using the implementation proposed in this patch, the flags effect is not changed (since it is not a runtime flag). However no error is reported, as the SetDaemonFlagRequestPB is using the force flag. (if the above understanding is correct) In the end I figured that I have to add startup flag support in the test suite. However since I already implemented this runtime flag setting support, I figured I submit the patch, as it still provides value for the test suite. I added docstrings to the functions, such that developers are aware, that the 'runtime' property of the flag is not verified by this function. -- To view, visit http://gerrit.cloudera.org:8080/19640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c381b207e2c0dbd700f94180e2311688be6f62c Gerrit-Change-Number: 19640 Gerrit-PatchSet: 5 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Sun, 02 Apr 2023 09:23:17 +0000 Gerrit-HasComments: Yes
