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

Should Registration Instances contain limited fields? #32

Closed
slominskir opened this issue Dec 22, 2021 · 3 comments
Closed

Should Registration Instances contain limited fields? #32

slominskir opened this issue Dec 22, 2021 · 3 comments
Labels
question Further information is requested

Comments

@slominskir
Copy link
Member

slominskir commented Dec 22, 2021

After reviewing the alarm registration data from our existing alarm system it seems we may not need many overridable fields in registration instances as the vast majority of the time it appears the class defaults will suffice. The remaining few cases could be addressed by creating additional classes, including some classes which may only have one instance. The trade-off may be simplicity vs flexibility. It would be much cleaner and easier to reason about if instances had very limited fields and slightly less clumsy for sync processes that automatically keep the subset of alarms that are configured elsewhere up-to-date (From the CED for example). Further, it is a little more clumsy for ops to make changes, because to be most confident in updates they'd actually need to review EVERY instance of a class anyways to ensure there are no accidental (or previously correct, but now out-dated) instance overrides of the fields they are updating (and harder to find). It may just be easier to invest the extra up-front cost to ensure that every alarm "type" is directly accounted for as an alarm class.

@slominskir slominskir added the question Further information is requested label Dec 22, 2021
@slominskir
Copy link
Member Author

slominskir commented Dec 22, 2021

The limited set of instance fields may be:

  • Name
  • Class
  • Producer (epicspv)
  • Location
  • Maskedby
  • Screenpath

That means classes may be solely responsible for:

  • Corrective action
  • Rationale
  • Priority
  • On-delay
  • Off-delay
  • Category
  • Point-of-Contact
  • Latching
  • Filterable

Note: The Name, Class, and Producer are already required and only found in the instance. The Location field is currently optional, and if not specified in the Instance, then inherited from the Class. We may re-consider this though and for simplicity just make Location always come from Instance. Special handling of one field may not be worth the complexity and awkward update process in which ops must check both class and instances anyway. Keep inheritance, but drop overrides

@slominskir
Copy link
Member Author

Note: Memory (space) considerations are probably negligible due to the relatively small amount of data. However AVRO encodes all fields even if they're null, and even if they're the default value, so it will actually save space to remove all the null valued fields from instances.

@slominskir
Copy link
Member Author

There is some existing data that suggests some fields must remain instance-level.

Examples where screenpath varies by instance here:

Examples where latching varies by instance here:

These cases should be examined for correctness and/or ability to fold into additional classes.

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

No branches or pull requests

1 participant