Add DataSourcingActor #52
Conversation
github-actions
bot
added
part:actor
shsms
force-pushed
the
30-data-sourcing-actor
branch
2 times, most recently
from
f74f173 to
b6a173b
Compare
leandro-lucarella-frequenz
left a comment
leandro-lucarella-frequenz
left a comment
There was a problem hiding this comment.
The microgrid source was a bit hard to follow, not sure if there is a simpler way to do it because I guess it has quite a bit of inherent complexity, but maybe more comments can be added. I feel I lacked a more high-level overview when reviewing about what are the responsibilities of each part, or the general algorithm, or even comments on the main attributes for microgrid source class.
shsms
left a comment
shsms
left a comment
There was a problem hiding this comment.
I will add some more comments tomorrow as well.
|
|
||
|
|
||
| @dataclass | ||
| class ComponentMetricRequest: |
There was a problem hiding this comment.
Both ComponentMetricId and ComponentMetricRequest need to be accessible for other actors, and will probably be accepted by the resampling actor too? So I put it in the frequenz/sdk/types.pyfrequenz/sdk/data_pipeline/types.py). Does that sound ok?
|
|
||
|
|
||
| @dataclass | ||
| class ComponentMetricRequest: |
There was a problem hiding this comment.
For me it doesn't sound like a general interface, I see it more owned by the data sourcing actor, even if other actors use it. It is completely coupled with how the data sourcing actor works and will have to be updated if the data sourcing actor interface changes, this is why I think they belong together.
Yes, probably the resampling actor will use it, but it can expose an alias, because eventually the resampling actor could have other parameters in the request (I was thinking that maybe, just maybe, we could want at some point to request a metric to be resampled with a particular resampling function after we talked about resampling a xxx_max metric for example). In that case it will have to define it's own request.
All that said, it is still a style issue I guess, so I won't oppose if you want to leave it like this. :)
| class DataSourcingActor: | ||
| """An actor that provides data streams of metrics as time series.""" | ||
|
|
||
| def __init__( | ||
| self, | ||
| request_receiver: Receiver[ComponentMetricRequest], | ||
| registry: ChannelRegistry, |
There was a problem hiding this comment.
we probably need a global singleton registry as well, before this PR can be merged.
There was a problem hiding this comment.
Why? I mean why it is a requirement for this to be merged?
There was a problem hiding this comment.
Because we're passing the registry as an argument to an actor, and that's not ideal.
We can of course merge, but then we'll have to come back and fix this afterwards.
sahas-subramanian-frequenz
force-pushed
the
30-data-sourcing-actor
branch
from
508e778 to
8d2f45c
Compare
shsms
force-pushed
the
30-data-sourcing-actor
branch
3 times, most recently
from
6f713b1 to
e0a563f
Compare
leandro-lucarella-frequenz
left a comment
leandro-lucarella-frequenz
left a comment
There was a problem hiding this comment.
LGTM, I can approve after the fixups are squashed (otherwise I will have to approve again anyway :)
shsms
force-pushed
the
30-data-sourcing-actor
branch
from
78521e8 to
ee5b6c6
Compare
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
shsms
force-pushed
the
30-data-sourcing-actor
branch
from
ee5b6c6 to
ebca3fa
Compare
3 participants
Closes #30