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
[ROBO-3381] split and merge poc #306
base: develop
Are you sure you want to change the base?
Conversation
a09d674
to
bb5a103
Compare
556ff81
to
52cf89e
Compare
edee40b
to
353521f
Compare
5a81556
to
9b8139c
Compare
aa6c8e5
to
bb3e9f5
Compare
HashSet<FlowMerge> allMerges = new(); | ||
foreach (var branch in split.Branches) | ||
{ | ||
var merges = GetMerges(branch).Distinct().ToList(); | ||
allMerges.AddRange(merges); | ||
} |
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.
You can simplify by getting all the merges for a split, e.g.:
var allMerges = GetMerges(split)
and GetMerges can be:
List<FlowMerge> GetMerges(FlowSplit flowSplit) { return ( from nodeInfo in _staticBranchesByNode where nodeInfo.Key is FlowMerge where nodeInfo.Value.GetTop().Contains(flowSplit) select nodeInfo.Key as FlowMerge ).ToList(); }
SourceDetail = new[] { node }.Concat(otherNodes ?? Array.Empty<FlowNode>()).ToArray() | ||
}); | ||
} | ||
List<FlowMerge> GetMerges(FlowNode flowNode) |
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.
Change to return the merges for a split
AddStack(preStacks.SplitsStack); | ||
} | ||
|
||
private void AddStack(List<List<FlowSplit>> stack) |
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.
Name proposal: AddUniqueStack
} | ||
|
||
private bool HasStack(List<FlowSplit> stack) | ||
=> SplitsStack.Any(b => stack.Count == b.Count && stack.All(p => b.Contains(p))); |
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.
=> SplitsStack.Any(s => s.SequenceEqual(stack));
private bool HasStack(List<FlowSplit> stack) | ||
=> SplitsStack.Any(b => stack.Count == b.Count && stack.All(p => b.Contains(p))); | ||
|
||
internal bool IsOnStack(StaticNodeStackInfo toConfirm) |
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.
Not needed, if you rewrite the GetMerges method
|
||
public void AddPop(StaticNodeStackInfo popFrom) | ||
{ | ||
var result = popFrom.SplitsStack.Select(s => Enumerable.Reverse(s).Skip(1).Reverse().ToList()).ToList(); |
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.
Quite difficult to understand
return new (SplitsStack.Select(b => b.LastOrDefault())); | ||
} | ||
|
||
public void Push(FlowSplit newSplit, StaticNodeStackInfo splitsStackInfo) |
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.
Naming: Push vs AddPop
Fx.Assert(completedInstance != null, "cannot request cancel if we never scheduled any children"); | ||
// we are done but the last child didn't complete successfully | ||
if (completedInstance.State != ActivityInstanceState.Closed) | ||
if (WasVisited(current)) |
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.
WasVisited is based on node.Flowchart != null.
I did not understand if the new nodes are created every time before CacheMetadata
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.
?
531bdcd
to
8b65fa6
Compare
7f54cf0
to
1247d51
Compare
No description provided.