-
Notifications
You must be signed in to change notification settings - Fork 90
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
Fix AlarmStateInitializer to store most recent kafka records from the alarm state topic #3079
Fix AlarmStateInitializer to store most recent kafka records from the alarm state topic #3079
Conversation
…test state records from kafka (if any) when first starting up
…n case of bad record key
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.
What you propose looks better, but while you're at it, how about this, which is overall shorter
for (final ConsumerRecord<String, String> record : records)
{
// Only handle state updates
if (record.key().startsWith(AlarmSystem.STATE_PREFIX))
{
final String path = record.key().substring(6);
...
}
}
+1 in addition to the small improvement in not having to create a new String object this is also much easier to read |
Thanks! Agreed that it is easier to read this way. Change made and confirmed it still works as expected. |
Also, one might wonder why "6" in here:
It's because This would be safer:
... but now one could argue that just "6" is faster than calling |
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.
Thanks, that looks better.
And if it actually works, that'd be perfect!
Thanks for looking into this! |
Fixes #3078
Change the end index of the substring() call to 6 so that it will be possible for
type
to equal theAlarmSystem.STATE_PREFIX
in the equality check.After I made this change, confirmed that acknowledgements do persist between alarm server reboots.
Although the change is minor this does cause this code block to be hit that wasn't possible before. Reading through the code it looks ok to me, and testing showed things seemed to be working as expected.
The eventual change this leads to outside this class is that initial_states will have something to pass to the
AlarmServerPV
here:phoebus/services/alarm-server/src/main/java/org/phoebus/applications/alarm/server/ServerModel.java
Line 394 in 6a6c5be