Adding preferred region option in ctl workload for DR drill #23115
Conversation
simplynaveen20
requested review from
FabianMeiswinkel,
aayush3011,
kirankumarkolli,
kushagraThapar,
mbhaskar,
milismsft,
moderakh and
xinlian12
as code owners
ghost
added
the
| private boolean manageDatabase = false; | ||
|
|
||
| @Parameter(names = "-preferredRegionsList", description = "Comma separated preferred regions list") | ||
| private String preferredRegionsList; |
There was a problem hiding this comment.
as a side, note, Configuration.java should hide all the config parsing capabilities.
see 3.1. Custom types - Single value section from the following list for how to pass a list and parse a list.
https://jcommander.org/
There was a problem hiding this comment.
Thanks for suggestion ,added the string to list converter
| if (preferredArray != null && preferredArray.length > 0) { | ||
| preferredRegions = new ArrayList<>(Arrays.asList(preferredArray)); | ||
| } | ||
| } |
There was a problem hiding this comment.
please factor out this code outside of the AsyncCtlWorkload.java to be used by all benchmark components.
| @Parameter(names = "-manageDatabase", description = "Control switch for creating/deleting underlying database resource") | ||
| private boolean manageDatabase = false; | ||
|
|
||
| @Parameter(names = "-preferredRegionsList", description = "Comma separated preferred regions list") |
There was a problem hiding this comment.
this is a useful config, as we are adding a global config to the benchmark tool, could you please pass this config to the other components in the benchmark tool as well? as we are adding the config to the main configuration we should support it everywhere. otherwise will be confusing when we use the non ctl workloads why this config doesn't work.
There was a problem hiding this comment.
I was debating it originally , now you mentioned that i added for all the workload. Thanks !
moderakh
left a comment
moderakh
left a comment
There was a problem hiding this comment.
LGTM. Thanks Naveen.
two minor comments:
-
this is a useful config, as we are adding a global config to the benchmark tool, could you please pass this config to the other components in the benchmark tool as well? as we are adding the config to the main configuration we should support it everywhere. otherwise will be confusing when we use the non ctl workload
-
as a side note, Configuration.java should hide all the config parsing capabilities.
see 3.1. Custom types - Single value section from the following list for how to pass a list and parse a list.
https://jcommander.org/
| } | ||
|
|
||
| private void createClients() { | ||
| private void createClients(Configuration cfg) { |
There was a problem hiding this comment.
We already have the configuration object in this class, why do we need to create a separate one ?
There was a problem hiding this comment.
It was the same object , however redundant reference , removed it now. Thanks
3 participants
Adding preferred region option in ctl workload for DR drill