-
Notifications
You must be signed in to change notification settings - Fork 487
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
Allow other nodes than the component nodes to have dependants #6225
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
ed31135
Allow other nodes than component nodes to be evaluated
wildum 2c9fb24
add comments to the func interface
wildum 21b13c0
use blocknode interface instead of the new NodeWithDependants interface
wildum d18b31f
refactor the way the LastUpdatedTime of a node is passed to the depen…
wildum 5196a6d
put back newlines
wildum File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package controller | ||
|
||
import ( | ||
"time" | ||
|
||
"github.com/grafana/agent/component" | ||
) | ||
|
||
// NodeWithDependants must be implemented by BlockNode that can trigger other nodes to be evaluated. | ||
type NodeWithDependants interface { | ||
BlockNode | ||
|
||
// LastUpdateTime returns the time corresponding to the last time where the node changed its exports. | ||
LastUpdateTime() time.Time | ||
|
||
// Exports returns the current set of exports from the managed component. | ||
Exports() component.Exports | ||
|
||
// ID returns the component ID of the managed component from its River block. | ||
ID() ComponentID | ||
} | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 there's a few issues with this interface:
The name is potentially misleading. The name implies it's about a characteristic of a particular node, but the description is more about whether a BlockNode can emit an event informing its dependents to be updated. In fact, from the perspective of the DAG, any node can have dependents regardless of whether it implements this interface.
The set of methods is perhaps too broad; it's not obvious why a node which can emit updates to cause other nodes to be evaluated needs to have component exports, or a component ID.
Because of the points above, I think it's hard to justify introducing this interface as a new concept to keep inside one's mental model of the controller code.
I would recommend that we aim to not introduce this interface, opting instead for the following:
LastUpdateTime()
to BlockNode instead. This would be a reasonable expectation of a BlockNode; as all BlockNodes support evaluating, it makes sense for them to also report their last update time. (Other methods should not be moved, just LastUpdateTime.)NodeWithDependents
, update it to accept aBlockNode
instead. You will need to add some type switching when a more concrete type is necessary.OnNodeWithDependantsUpdate
is a bit harder to read thanOnBlockNodeUpdate
.WDYT?
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.
Makes sense, I was not really happy with the name in the first place so I'm happy to change this :)
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 made the changes to see how it looks but after putting more thought into it I'm not 100% happy with it:
But for now the definitions of Arguments and Exports are really tied to components and not nodes so it would require bigger changes.
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.
Ah, I see. Honestly, I don't think a node should have to track its own export time anyway; the controller could be tracking that when
OnBlockNodeUpdate
is called. What do you think? Should that be scoped into this PR so we can take it out of the interface?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.
Sounds good, the change is small and the time is only used by one metric. I think that it is worth doing it in the PR as it prevents introducing redundant code. I will take care of it