Do not have local ccdb URL parameters in configKeyValues initialized from NameConf#7814
Merged
davidrohr merged 1 commit intoAliceO2Group:devfrom Dec 5, 2021
Merged
Conversation
shahor02
approved these changes
Dec 5, 2021
Collaborator
shahor02
left a comment
There was a problem hiding this comment.
Hi @davidrohr
Right, did not think about that. We still can have a possibility to have set e.g. DigiParams::ccdb and NameConf::mCCDBServer to different servers by setting the CCDB strings in all ConfigurableParams except NameConf::mCCDBServer to "" and adding methods like
const auto& DigiParams::getCCDB() {
return ccdb.empty() ? o2::base::NameConf::getCCDBServer() : ccdb;
}
Collaborator
Author
|
Yes, sure, we can do that. Shall I change this PR accordingly?
|
Collaborator
|
Not sure we really need this, these few configurables which you have fixed had CCDB setting because there was no central one rather because they need special settings. I would leave this PR as is, once we switch to DPL ccdb server, we will have a possibility to set even each object server individually. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@shahor02 : This scheme of having one ConfigurableParam initialized from another one is conceptionally broken. It becomes a race condition which one is populated first, thus it is not well defined if the default value is taken, or the value after parsing the
--configurableParamoption.This PR does away with ccdb settings in other configurableParams and uses the NameConf::CCDBServer for all.
I am not sure if this is the correct thing to do. It basically means that within one processor one cannot access different CCDB servers. But I don't see another way right now that is somewhat clean.
This will hopefully be solved once we have the proper CCDB API.
@sawenzel : In general, perhaps we should add a protection that no instance can be created before the parsing has happened?
Some days ago I also had a similar problem that someone was caching a value from a ConfigurableParam in a static variable, and that static variable was initialized at application startup time from the instace before the command line arguments were passed, so it hold the default value, and I could not set it via --configKeyValues. We should avoid such misleading behavior.