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

Fixes #19380: make use of cache more efficient in NodeInfoService #3661

Conversation

ncharles
Copy link
Member

@ncharles ncharles commented Jun 2, 2021

@ncharles ncharles requested a review from fanf June 2, 2021 19:53
@ncharles ncharles added Trigger test WIP Use that label for a Work In Progress PR that must not be merged yet labels Jun 2, 2021
Fixes #19380: make use of cache more efficient in NodeInfoService
@ncharles
Copy link
Member Author

ncharles commented Jun 2, 2021

PR updated with a new commit

@@ -93,8 +93,8 @@ final case class NodeInfo(
val isSystem = node.isSystem
val isPolicyServer = node.isPolicyServer
val creationDate = node.creationDate
val nodeReportingConfiguration = node.nodeReportingConfiguration
val properties = node.properties
def nodeReportingConfiguration = node.nodeReportingConfiguration
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment, else it will be lost the nexttime we touch that code

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not definitive, i'm only testing stuff

@ncharles
Copy link
Member Author

ncharles commented Jun 3, 2021

Do not merge, this is tests

…nfoService

Fixes #19380: make use of cache more efficient in NodeInfoService
@ncharles
Copy link
Member Author

ncharles commented Jun 3, 2021

PR updated with a new commit

@@ -375,7 +375,7 @@ trait NodeInfoServiceCached extends NodeInfoService with NamedZioLogger with Cac
TimingDebugLogger.debug(s"Getting node info entries: ${t1-t0}ms")

for {
res <- ZIO.foreach(nodes) { case (id, nodeEntry) => mapOneNode(id, nodeEntry, nodeInventories, machineInventories) }
res <- ZIO.foreachPar(nodes) { case (id, nodeEntry) => mapOneNode(id, nodeEntry, nodeInventories, machineInventories) }
Copy link
Member Author

Choose a reason for hiding this comment

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

this is a great improvement

Copy link
Member Author

Choose a reason for hiding this comment

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

this works too well and prevent policy generation to go fast :/

Copy link
Member

Choose a reason for hiding this comment

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

you likely need to limite the parallelism with foreachParN(maxParallism)(nodes) { .... }

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but i don't know which value to set

…n NodeInfoService

Fixes #19380: make use of cache more efficient in NodeInfoService
@ncharles
Copy link
Member Author

ncharles commented Jun 3, 2021

PR updated with a new commit

…cient in NodeInfoService

Fixes #19380: make use of cache more efficient in NodeInfoService
@ncharles
Copy link
Member Author

ncharles commented Jun 3, 2021

PR updated with a new commit

…re efficient in NodeInfoService

Fixes #19380: make use of cache more efficient in NodeInfoService
@ncharles
Copy link
Member Author

ncharles commented Jun 3, 2021

PR updated with a new commit

…ache more efficient in NodeInfoService

Fixes #19380: make use of cache more efficient in NodeInfoService
@ncharles
Copy link
Member Author

ncharles commented Jun 3, 2021

PR updated with a new commit

…se of cache more efficient in NodeInfoService

Fixes #19380: make use of cache more efficient in NodeInfoService
@ncharles
Copy link
Member Author

ncharles commented Jun 3, 2021

PR updated with a new commit

@ncharles
Copy link
Member Author

ncharles commented Jun 9, 2021

this one is not what it says - it will need rework

@ncharles ncharles marked this pull request as draft June 9, 2021 19:54
@ncharles ncharles closed this Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Trigger test WIP Use that label for a Work In Progress PR that must not be merged yet
Projects
None yet
2 participants