Skip to content

[QC-443] Represent the whole configuration file in C++ structures#736

Merged
knopers8 merged 5 commits into
AliceO2Group:masterfrom
knopers8:refactoring
Jul 21, 2021
Merged

[QC-443] Represent the whole configuration file in C++ structures#736
knopers8 merged 5 commits into
AliceO2Group:masterfrom
knopers8:refactoring

Conversation

@knopers8
Copy link
Copy Markdown
Collaborator

@knopers8 knopers8 commented Jun 22, 2021

This plans to address a number of issues:

  • Risk of having different default values in different places in code which read the same thing
  • No central place to define the configuration scheme (there is documentation, but it follows the format, not defines),
  • QC Data Processors are aware of the configuration file structure
  • No way to run QC without config files
  • Many calls to consul during one initialization (assuming that one ConfigurationFactory::create call always calls consul/opens a file). Notice that e.g. each Check creates a ConfigurationInterface by itself.
  • We can't retrieve configuration from DPL, because we use a mix of ConfigurationInterface and ptree. It should be fine if we always work on a raw ptree.
  • InfrastructureGenerator becomes a mess, because we read, interpret and generate infrastructure there. This should move reading out of this class (and interpreting partially).

I worked on top of another PR to avoid merge conflicts in the future. Please click on the "initial progress on refactoring" commit to get only the relevant changes.

TODO:

  • Make TaskRunner avoid using ConfigurationInterface completely, if possible.
  • Prepare Specs for other QC actors and read them accordingly. It could be divided into many PRs.

@knopers8 knopers8 requested a review from Barthelemy June 22, 2021 10:09
@knopers8
Copy link
Copy Markdown
Collaborator Author

@Barthelemy Could you please have a look before I further progress here? I would like to avoid getting into some dead end or missing some use-case that I could accidentally forbid.

Copy link
Copy Markdown
Collaborator

@Barthelemy Barthelemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. It is an ambitious endeaviour !
I like the idea and the indirection you introduce with the *Spec vs *Config. Actually, I might have missed something but why is there no need for a GlobalSpec ?

I don't see anything fundamentally wrong or missing. You can continue with the implementation.

@knopers8
Copy link
Copy Markdown
Collaborator Author

Thanks.
I used directly GlobalConfig, because this part does not need any clever translation between Specs and Configs. Though perhaps I should keep it uniform and call it GlobalSpec anyway, you never know how it will evolve.

…others

Now TaskRunner only gets TaskRunnerConfig with all the information it needs in order to run.
It will be able to update the configuration by retrieving it from DPL and letting TaskRunnerFactory provide updated TaskConfig.
@knopers8 knopers8 changed the title [WIP] [QC-443] Represent the whole configuration file in C++ structures [QC-443] Represent the whole configuration file in C++ structures Jul 20, 2021
@knopers8 knopers8 requested a review from Barthelemy July 20, 2021 07:28
@knopers8
Copy link
Copy Markdown
Collaborator Author

The first part of refactoring should be ready, the future PRs will include a similar changes for Data Sampling, Checks, Aggregators, etc.

Copy link
Copy Markdown
Collaborator

@Barthelemy Barthelemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for me, very nice I must say.

Comment thread Framework/CMakeLists.txt Outdated
Comment thread Framework/src/InfrastructureSpecReader.cxx Outdated
@knopers8 knopers8 merged commit b479f27 into AliceO2Group:master Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants