Skip to content

Fix wrong warnings about bad flags assigned to good qualities #2583

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.

Sign up for GitHub

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

Merged
knopers8 merged 1 commit into from
Jul 31, 2025
Merged

Fix wrong warnings about bad flags assigned to good qualities #2583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Framework/src/AggregatorRunner.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ AggregatorRunner::QualityObjectsWithAggregatorNameVector AggregatorRunner::aggre
ILOG(Info, Devel) << "Processing aggregator: " << aggregatorName << ENDM;

if (mUpdatePolicyManager.isReady(aggregatorName)) {
ILOG(Info, Devel) << " Quality Objects for the aggregator '" << aggregatorName << "' are ready, aggregating" << ENDM;
ILOG(Info, Devel) << " Quality Objects for the aggregator '" << aggregatorName << "' are ready, aggregating" << ENDM;
auto newQOs = aggregator->aggregate(mQualityObjects, *mActivity); // we give the whole list
mTotalNumberObjectsProduced += newQOs.size();
mTotalNumberAggregatorExecuted++;
Expand Down
6 changes: 3 additions & 3 deletions Modules/Common/src/TrendCheck.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,12 +345,12 @@ Quality TrendCheck::check(std::map<std::string, std::shared_ptr<MonitorObject>>*
Quality result = mQualities.empty() ? Quality::Null : Quality::Good;
for (auto& [key, quality] : mQualities) {
(void)key;
for (const auto& flag : quality.getFlags()) {
result.addFlag(flag.first, flag.second);
}
if (quality.isWorseThan(result)) {
result.set(quality);
}
for (const auto& flag : quality.getFlags()) {
result.addFlag(flag.first, flag.second);
}
Copy link
Collaborator

@justonedev1 justonedev1 Jul 31, 2025

Choose a reason for hiding this comment

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

I am sorry but it isn't obvious for me how just reversing order of these two operations in one loop would fix the issue. you are creating Quality object, setting it's quality to the one you find as worse (finding the worst) ... aka setting it's level and name... and than adding all flags from all of the qualities encountered in check. So unless I am missing something I don't understand how you achieve with different situation than before.

Copy link
Collaborator

@justonedev1 justonedev1 Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

nevermind, I understand, there are internal checks in addFlag ... but just reading the PR isn't obvious why this should fix anything. I would expect that addFlag is just adding flag without any internal checks.

Copy link
Collaborator Author

@knopers8 knopers8 Jul 31, 2025

Choose a reason for hiding this comment

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

I think it's quite normal that functions validate their inputs, but indeed I could have mentioned where the warning was coming from.

}

return result;
Expand Down
6 changes: 3 additions & 3 deletions Modules/Common/src/WorstOfAllAggregator.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,12 @@ std::map<std::string, Quality> WorstOfAllAggregator::aggregate(QualityObjectsMap
Quality current = Quality::Good;
for (const auto& [qoName, qo] : qoMap) {
(void)qoName;
for (const auto& flag : qo->getFlags()) {
current.addFlag(flag.first, flag.second);
}
if (qo->getQuality().isWorseThan(current)) {
current.set(qo->getQuality());
}
for (const auto& flag : qo->getFlags()) {
current.addFlag(flag.first, flag.second);
}
}
ILOG(Info, Devel) << "Aggregated Quality: " << current << ENDM;

Expand Down