Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19640 )
Change subject: [Python] Support setting runtime flags in tests ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/19640/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19640/7//COMMIT_MSG@7 PS7, Line 7: runtime What's the semantics of 'runtime' here? As I can see, it turned out that the newly added functions set any type of flag, whether tagged as 'runtime' or not. With that, maybe drop 'runtime' part everywhere, so at least it would have similar names to what's available in C++ test harness? What do you think? http://gerrit.cloudera.org:8080/#/c/19640/7//COMMIT_MSG@9 PS7, Line 9: Currently it is not streamlined, how to set Kudu master, tserver runtime : flags in Python tests Currently, setting Kudu master's and tserver's flags in Python tests is not streamlined. http://gerrit.cloudera.org:8080/#/c/19640/7//COMMIT_MSG@19 PS7, Line 19: . (see : ControlShellResponsePB) nit: misplaced period http://gerrit.cloudera.org:8080/#/c/19640/7//COMMIT_MSG@25 PS7, Line 25: . (these indicate a proper turnaround from the : server) nit: misplaced period http://gerrit.cloudera.org:8080/#/c/19640/7/python/kudu/tests/common.py File python/kudu/tests/common.py: http://gerrit.cloudera.org:8080/#/c/19640/7/python/kudu/tests/common.py@134 PS7, Line 134: a master >From the implementation, it seems this methods sets the flag for all the >masters in the cluster, so this description looks a bit confusing, no? Also, the name of the method looks like is a misnomer for the same reason as well. Same for the new method below. 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@47 PS3, Line 47: with self.assertRaisesRegex(Exception, error_msg): > In the above example 'check_expired_table_interval_seconds' is not a runtim Yup, I meant adding a small case to try setting a non-runtime flag to a valid value. In other words, the idea was to have a scenario to track regressions in NOT_SAFE, but I wasn't aware that mini-cluster always adds 'force'. OK, then this looks good enough. -- 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: 7 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: Tue, 04 Apr 2023 05:57:27 +0000 Gerrit-HasComments: Yes
