-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fixes #19248: Empty /var/rudder/policy-generation-info/node-configuration-hashes.json after a policy generation that changed nothing #3637
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the filterNot is overcomplicating the method
|
||
sequential | ||
|
||
"when nothing was written, we should get an empty succe" >> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"when nothing was written, we should get an empty succe" >> { | |
"when nothing was written, we should get an empty success" >> { |
else semaphore.withPermits(1) { | ||
for { | ||
current <- nonAtomicRead() | ||
filtered = current.hashes.filterNot(h => nodeIds.contains(h.id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm unsure tht you need this one: you are traversing current.hashes , and n times the nodeIds
doing simply (filtered ++ hashes).toList.sortBy(_.id.value) would yield the same result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, filtered
is a list, not a map, and hashes
is not a map either, so I will have several node config hashes for the same node Id.
I would need to do current.hashes.map(h => (h.id, h)).toMap
, same for hashes
, so two traverse, and one more (more coslty) for the map merge.
PR updated with a new commit |
This PR is not mergeable to upper versions. |
OK, squash merging this PR |
…tion-hashes.json after a policy generation that changed nothing
35e923c
to
1a51078
Compare
https://issues.rudder.io/issues/19248