Skip to content
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

Discussion about the message models #3

Open
gvannoye opened this issue Jun 3, 2021 · 3 comments
Open

Discussion about the message models #3

gvannoye opened this issue Jun 3, 2021 · 3 comments

Comments

@gvannoye
Copy link

gvannoye commented Jun 3, 2021

As a reminder, the message formats are described in the task list:

I have some questions and remarks concerning the SNEWSObservation model:

  1. Should we use only one dataclass for the three tiers of message? I believe it is difficult for a dataclass to have optional fields, so that would mean leaving some fields blank when sending the message. If we do so, do we add an identifier for the tier of the message?
  2. When creating a SNEWSObservation object, a message_id field is required. The documentation indicates that it is “for the purpose of recording and tracing messages for SNEWS usage”, but this raises the question: should every id of every message be unique?
    If so that would mean that every experiment should listen to the SNEWSOBSERVATION topic and record the ids to avoid duplicates, in this case an id that is given server side would surely be more efficient.
    Another possibility is that the id is unique for a given experiment (meaning that experiments need to keep track of the number of alerts they sent).
  3. I believe letting each field containing an arbitrary string is a bad idea (perhaps it was done that way waiting for the decision of the proper format). By restricting what the model can be, errors can be avoided and the handling of message should be easier server-side.
    One possibility is to verify each string (either by verifying they are in a list of authorized strings, or by parsing them to verify the format) at the creation of the object.
@habig
Copy link

habig commented Jun 3, 2021

Tying to summarize the conversation about this right now. We do need a message ID field in the string, so that we can map the incoming data to an object, check that it's complete data for that object type, etc. Those objects should indeed be carefully defined.

This is not yet the case because the simple coincidence use case (SNEWS 1.0) we're trying to get deployed now does just one thing. However, as was pointed out yesterday by Andrey, we actually already need the ability to have a "retract/update" message: to implement that, we already need a second message ID.

The first step here would be to add this layer of "stream with id -> appropriate object" for the existing coincidence case, even though we have only one thing. Check that this works, then we can extend it to the next case: could be the update/retract object. Or, as we also just discussed, could be a "high rate/low rate" object, if we want to replicate the SNEWS1 two-tier system over the same channel. Then there are two different deciders consuming each incoming set of objects.

@habig
Copy link

habig commented Jun 3, 2021

Thinking a little more: the sending and receiving side of things should each use the same object definition, written as a class that has methods for writing itself out to a hop stream and reading itself in from a hop stream. That will minimize errors.

@myNameIsPatrick
Copy link
Collaborator

I believe letting each field containing an arbitrary string is a bad idea (perhaps it was done that way waiting for the decision of the proper format). By restricting what the model can be, errors can be avoided and the handling of message should be easier server-side.
One possibility is to verify each string (either by verifying they are in a list of authorized strings, or by parsing them to verify the format) at the creation of the object.

It should be fairly straightforward to add client-side validation within these message models in its __post_init__ method as one path forward. Since the messages are JSON, jsonschema could be one way of doing this including restricting the types of strings allowed for specific fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants