Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Printing log when we find a default CCDB entry #8659

Merged
merged 2 commits into from
Apr 27, 2022

Conversation

chiarazampolli
Copy link
Collaborator

@shahor02 , @ktf , is this the correct place? From a local test it works, but I don't know if it covers all possibilities.
I would allow the three keys "default", "Default", "DEFAULT", let me know what you think.

@chiarazampolli chiarazampolli requested a review from a team as a code owner April 26, 2022 11:15
@shahor02
Copy link
Collaborator

@chiarazampolli the place is correct, but I would not consider different cases, just default (can be added https://github.com/pnwkw/AliceO2/blob/f5d53e06caa8d9224dcbe7d0bcb6e0d01af8c63f/Common/Utils/include/CommonUtils/NameConf.h#L69 and used for uploading). Also, I would not check the true, 1 etc.: non-default objects should not have this tag at all, just presence of default should be enough. So, I would change it simply to:

if (headers.find("default")!=headers.end()) {
  LOGP(info, "******** Default entry used for {} ********", path);
}

@chiarazampolli
Copy link
Collaborator Author

Ok, I can remove the various options for writing "default", but what if someone used "default = false" to specify that it is not a default? I would keep the check on the value.

@shahor02
Copy link
Collaborator

Who can use it? The default objects should be uploaded manually and this can be done by special operators only. If we can avoid passing the value at all, I would prefer this.

@chiarazampolli
Copy link
Collaborator Author

Ok, I will change it.

@chiarazampolli
Copy link
Collaborator Author

Done.

@@ -211,6 +212,10 @@ auto populateCacheWith(std::shared_ptr<CCDBFetcherHelper> const& helper,
// FIXME: I should send a dummy message.
continue;
}
// printing in case we find a default entry
if (headers.find("default") != headers.end()) {
LOGP(info, "******** Default entry used for {} ********", path);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LOGP(info, "******** Default entry used for {} ********", path);
LOGP(detail, "******** Default entry used for {} ********", path);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the effect of "detail"?

Copy link
Member

Choose a reason for hiding this comment

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

It's like info, but it does not go to infologger.

@@ -23,6 +23,7 @@
#include <TError.h>
#include <TMemFile.h>
#include <functional>
#include <boost/algorithm/string/predicate.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need predicate?

Copy link
Collaborator Author

@chiarazampolli chiarazampolli Apr 26, 2022

Choose a reason for hiding this comment

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

It is indeed not needed anymore, after the changes following Ruben's comments, I will remove it. Thanks for spotting.

@shahor02
Copy link
Collaborator

@chiarazampolli I've checked, the metadata with empty value does not work, so we have to provide something but should look on the presence of the tag only.

@chiarazampolli
Copy link
Collaborator Author

Ciao,

Yes, I had not understood that you proposed to not add the value at all. We should just make sure that the value is not misleading when we do the manual upload.
Anyway, for the changes here, it does not matter.
Chiara

@chiarazampolli chiarazampolli enabled auto-merge (squash) April 27, 2022 07:06
@chiarazampolli
Copy link
Collaborator Author

If there are no more comments, can this be approved?

@shahor02 shahor02 disabled auto-merge April 27, 2022 07:12
@shahor02 shahor02 merged commit f153b1e into AliceO2Group:dev Apr 27, 2022
@chiarazampolli chiarazampolli deleted the printDefCCDB branch May 16, 2022 08:02
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.

3 participants