-
Notifications
You must be signed in to change notification settings - Fork 23
[O2B-1475] Add proto message and associated ones for BeamModeEvent changes #752
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
Conversation
| * Beam mode changes are propagated as Kafka events and to be sent by the BKP-LHC-Client on a dedicated topic | ||
| * e.g. dip.lhc.beam_mode | ||
| */ | ||
| message Ev_BeamModeEvent { |
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.
This name Ev_BeamModeEvent follows the naming convention for the test of events.
I personally, consider that the Ev already stands for Event and I would recommend something like Ev_BeamModeChange
What do you @knopers8 and @justonedev1 think?
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 that you are right, but I would keep the same convention as the rest of ECS. So I'd keep it as it is now.
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.
Even though I also don't see a strong reason for the duplicated "event", I would rather keep it consistent with the existing convention.
|
Just a note: This PR needs to generate go representation of protofiles and @graduta asked us to do so, after we agree on this PR. |
justonedev1
left a comment
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.
It looks okay for me, let's see what Piotr thinks as he is the one who implements this part. One remark: we need to generate pb.go files and documentation from these (as already mentioned in distinct documentation)
PR which:
events.protothat is to be used by BKP-LHC-Client to send events related to STABLE_BEAMS changes which are to be consumed by ECS;common.protofor beam information that has currently been decided as needed by ECS