elastic / rally

Macrobenchmarking framework for Elasticsearch
Apache License 2.0
1.95k stars 313 forks source link

Add props to prevent typos in config properties #1795

Open sakurai-youhei opened 11 months ago

sakurai-youhei commented 11 months ago

This PR will eliminate numerous strings around config by typo-proof enums.

In short, cfg.opts("benchmarks", "local.dataset.cache") will be rewritten to cfg.opts(*props.BENCKMARKS.LOCAL.DATASET.CACHE)

The following points are aimed in the implementation:

  1. Not require any changes in section and key names.
  2. Support auto-completion by IDEs.
  3. Keep the existing code width the same as possible.
  4. Prepare the way to describe config properties in the code.
  5. Enable to express both dot-notated sections and keys.
  6. Make unpreferable unpacking fail on properties in the middle of dot notation. -> i.e. With props.SECTION.DOT.NOTATED.KEY, func(*props.SECTION.DOT.NOTATED) raises TypeError.

In return, the following restrictions are introduced:

Closes #1646

TODOs:

sakurai-youhei commented 10 months ago

@gbanasiak Thank you for your comment.

However, I became a bit suspicious that this hacky code is sustainable, even though I opened this PR. Also, if you plan to convert dot notations (list.config.option) to underscores (list_config_option), there might be a more straightforward way. Another thought is about another PR https://github.com/elastic/rally/pull/1798, which is probably more acceptable for everyone.

What do you think? Can we archive (close without merging and leave) this PR until this enum constants gets really crucial?

gbanasiak commented 10 months ago

@sakurai-youhei Thank you for iterating.

I think https://github.com/elastic/rally/pull/1798 is a step in the right direction. It will help working on https://github.com/elastic/rally/pull/1795 or similar, and tighten existing use of configuration, but by itself will not fully address https://github.com/elastic/rally/issues/1646 due to the following:

This wasn't stated explicitly but I've assumed we've embarked on the following journey:

I've already got accustomed to https://github.com/elastic/rally/pull/1795 logic and don't find it hacky, but if you see a more straightforward solution we can pursue this instead. We already looked at 2 proposals earlier in https://github.com/elastic/rally/issues/1646, and both were more repetitive in nature than this PR.

We can obviously also close this draft if you don't want to invest more time in it. Enum constants are not a top priority. It is a nice to have feature of the code. Also, it is understood community contributions are not "guaranteed". It's your free time after all.

The switch from dot notation to underscores is an option for the future, but I don't think we want to introduce such breaking change now. I was just confirming this proposal is flexible enough to accommodate this path, I think it is.

sakurai-youhei commented 10 months ago

@gbanasiak Thank you for your comment.

[1]

once all literals replaced with props, switch Config methods to accept props only instead of section/key strings.

The above didn't come up in my mind, but that's a good idea indeed.

[2]

The switch from dot notation to underscores is an option for the future, but I don't think we want to introduce such breaking change now. I was just confirming this proposal is flexible enough to accommodate this path, I think it is.

Thinking about it over a long period, the answer is YES. The props can be reworked when the time will have come. I thought the switch would happen in the near future. But if it's not, I don't have anything other than this PR to achieve the DRY as of now.

[3]

I've already got accustomed to #1795 logic and don't find it hacky

That's good to know. :) OK, I will resume working on this PR once #1798 gets done.