-
Notifications
You must be signed in to change notification settings - Fork 96
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
Consistent Alarm naming convention between Alarm Table and Alarm Log Table #3242
base: master
Are you sure you want to change the base?
Consistent Alarm naming convention between Alarm Table and Alarm Log Table #3242
Conversation
Broadly it looks good... and it is a great effort to standardize across these applications/services Is there a reason to choose PV Message vs keeping it PV Status... is this to link it to the other instances where Current Message also maps to PV Message
|
We (You, Kay, and me) decided to use Message instead of Status to keep the modern EPICS convention which exists as far as I remember. Please correct me if I am wrong, since I am old and don't remember very well now. ;) |
@shroffk when I took the screen capture, I saw there is inconsistent. I didn't have much attention about the query itself. Can you check what kind of query strings will be changed due to the new naming convention? I will work with Sang-il to check them independently also. And I converted this PR to the draft. |
@shroffk it turns out, we need to change all variable names between Alarm Server and Alarm Logger table within multiple java codes. We will come back after all relevant codes changes, and the corresponding tests. |
Agree. In the alarm log table, did the query always support "message=...¤t_message=..." or is that a change from previously using "status...current_status..."? As for "current" vs. "pv", the labels in tables etc. would now use "PV Severity", "PV Message", no more "Current ..", but would the log table still use "current_message=..." for the query? On one hand, that should become "pv_message=..." to be consistent, but that's more than just a cosmetic label, it's changing the query language. |
I think changing the search key words for the alarm log table would mean we would have to update the elastic index mapping and then migrate the data. |
That seems a bit much. Still, seeing "pv .." in all the GUIs but having to use "current_.." in the query can also be confusing. Would it make sense to hack this by patching the query, |
I will come back when we have a better idea of how the query (new naming) works with elasticsearch and a better solution for consistent naming. I still think this is valuable, but I also agree that this seems a bit far from my original intention. |
@shroffk and @kasemir
As we've discussed and agreed, here is the pull request to contain the consistent and meaningful naming convention within the Alarm Table and the Alarm Log Table.
Here is the table which we've developed so far.
Please let me know what you think.
@shroffk we also have to work together to update the alarm documentation to reflect these changes.
Thanks,
@jeonghanlee, Soo Ryu, and @Sangil-Lee at ALS-U Controls, LBNL.