Fixes #20758: Improve node computation #4172
Conversation
ncharles
added
the
WIP
|
Do not review, still a wip |
| query.returnType match { | ||
| case NodeReturnType => | ||
| // we have a special case for the root node that always never to that group, even if some weird | ||
| // scenario lead to the removal (or non addition) of them | ||
| val withoutServerRole = inNodes.filterNot { case QueryResult(e, inv, _) => e(A_NODE_UUID) == Some("root") } | ||
| val withoutServerRole = foundNodes.filterNot { case x: NodeId => x.value == "root" } |
There was a problem hiding this comment.
It should no't be root, it will miss all relays!
| case NodeReturnType => | ||
| // we have a special case for the root node that always never to that group, even if some weird | ||
| // scenario lead to the removal (or non addition) of them | ||
| foundNodes.filterNot { case x: NodeId => x.value == ("root") } |
There was a problem hiding this comment.
same here, it cannot be only root
ncharles
force-pushed
the
bug_20758/improve_node_computation
branch
from
e8c0358
to
ef432ce
Compare
|
PR updated with a new commit |
|
PR updated with a new commit |
ncharles
removed
the
WIP
| @@ -875,6 +876,10 @@ trait NodeInfoServiceCached extends NodeInfoService with NamedZioLogger with Cac | |||
| def getNodeInfos(nodeIds: Set[NodeId]): IOResult[Set[NodeInfo]] = withUpToDateCache(s"${nodeIds.size} nodes infos") { cache => | |||
| cache.filter(x => nodeIds.contains(x._1)).values.map( _._2).toSet.succeed | |||
| } | |||
|
|
|||
| def getNodeInfosSeq(nodeIds: Seq[NodeId]): IOResult[Seq[NodeInfo]] = withUpToDateCache(s"${nodeIds.size} nodes infos") { cache => | |||
| nodeIds.map(id => cache(id)).map(_._2).succeed | |||
There was a problem hiding this comment.
it could fail if some requested node id are not in cache (for some inconsistency from somewhere).
What is the correct thing to do on missing nodeId ? Fail everything or ignore ?
If fail, something like:
ZIO.foreach(nodeIds) { id => cache.get(id).map(_._2).notOptionnal(s"node with id ${id.value} not in cache, likely an error")} }
If ignore:
nodeIds.map(id => cache.get(id).map(_._2)).collect { case Some(info) => info }.succeed
}```
There was a problem hiding this comment.
ha, very good point.
to be consistent, we should ignore, so the second approach is the right one. Thank you !
| } else { | ||
| foundNodeInfos.filter { nodeinfo => predicate(nodeinfo, predicates, composition, None) } | ||
| foundNodeInfos.flatMap { case nodeinfo => if (predicate(nodeinfo, predicates, composition, None)) { Some(nodeinfo.id) } else None } |
There was a problem hiding this comment.
can be directly
foundNodeInfos.collect { case nodeinfo if (predicate(nodeinfo, predicates, composition, None)) => nodeinfo.id }
There was a problem hiding this comment.
actually, it could still be the filter
There was a problem hiding this comment.
the collect is better, it avoids one creation of collection
|
PR updated with a new commit |
|
This PR is not mergeable to upper versions. |
|
OK, squash merging this PR |
fanf
changed the title
fanf
force-pushed
the
bug_20758/improve_node_computation
branch
from
68904dd
to
adc1b18
Compare
Based on #4164
Changing the types to remove most of the hashCode computation, most notably, using Set for NodeInfo is a really bad idea
With this (unfinished) code, computation of dynamic group goes from 1m15 (pr 4164) to 35s
Do not merge for now, this is only exploration
The only relevant thing is to return nodeId rather than NodeInfos now that we don't need serverrole info