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

Archive datasource #3034

Merged
merged 26 commits into from
Jun 27, 2024
Merged

Archive datasource #3034

merged 26 commits into from
Jun 27, 2024

Conversation

shroffk
Copy link
Member

@shroffk shroffk commented Jun 3, 2024

This is a WIP in preparation of the aa datasource project for the upcoming codeathon

@shroffk shroffk requested a review from georgweiss June 3, 2024 18:29
@@ -0,0 +1,21 @@
Alarm Datasource
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Archive, not Alarm Datasource

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed this.. thanks

@shroffk
Copy link
Member Author

shroffk commented Jun 3, 2024

This PR includes a restructuring of the databrowser application.

I wanted to move the archive reader bits out of the databrowser app since there are a few use cases where we see the archive reader being used without needing the UI bits. The foremost of these use cases is the archive datasource which is going to be used in the Save and Restore service and potentially also via the PVWS.

I think there might be more ideas to consider on how to restructure this...should each archiver have its reader in a standalone module, how much of the utility stuff should be move to core modules ( I already moved some of the Timestamp support classes to core-util), etc... I will add this PR as a discussion topic for next weeks codeathon

Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Preferences class needs to change:
AnnotatedPreferences.initialize(Activator.class, Preferences.class, "/appliance_datasource_preferences.properties");
to
AnnotatedPreferences.initialize(Preferences.class, "/appliance_datasource_preferences.properties");
otherwise setting in custom ini file will be ignored.

Looks like preference archive_url supports but one URL, unlike preference for data browser. At ESS we currently use four URLs.

Maybe the doc file should state supported time formats, and also clarify that the time specifier should be unquoted.

Seems time span is not (yet) supported?

@shroffk
Copy link
Member Author

shroffk commented Jun 4, 2024

I had fixed the preference but forgot to commit.

I believe that the documentation needs a lot more work... I hoping this to be accomplished during the codeathon

If we want to support multiple then we will have to have a system of selecting/setting one by default and then having some parameter to select from the rest... or should we let the user define any URL?

@shroffk shroffk marked this pull request as ready for review June 25, 2024 15:32
@shroffk
Copy link
Member Author

shroffk commented Jun 25, 2024

Currently,
I restructured this so that all the archive readers are still in one common module.
@kasemir @georgweiss does it make sense to split this too? should we have one dedicated module for each of the archive readers?

@kasemir
Copy link
Collaborator

kasemir commented Jun 25, 2024

At this time I see no advantage in splitting it further. Just adds more build steps.

@shroffk
Copy link
Member Author

shroffk commented Jun 25, 2024

Ok,
So we keep this PR as it is.
In the future if we want to clean up the dependencies in our individual products we can revisit this.

Copy link
Collaborator

@georgweiss georgweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, really nice feature.
Replay can easily be tested in probe. However, replay seems to push updates as "quickly" as possible. Should not time stamps be used to replay at the pace events were stored in the archiver?

@shroffk
Copy link
Member Author

shroffk commented Jun 27, 2024

ideally yes, the rate at which the updates are "replayed" still needs work, but I think I would like to merge this and spin that out to another PR :)

@kasemir
Copy link
Collaborator

kasemir commented Jun 27, 2024

Should not time stamps be used to replay at the pace events were stored in the archiver?

Great idea! Should also be applied to the data browser:
If user selects the last year as the time range, the plot will populate sample by sample over the course of one year. That way, you can most realistically re-live what happened over time ;-)

@georgweiss
Copy link
Collaborator

georgweiss commented Jun 27, 2024

the plot will populate sample by sample over the course of one year.

In the meanwhile we can spend time in developers' meetings.

@shroffk
Copy link
Member Author

shroffk commented Jun 27, 2024

Well... the idea would be to have a rate/multiplier so basically it would be replay that hour I am interested in at 50x or 10x etc...

@kasemir
Copy link
Collaborator

kasemir commented Jun 27, 2024

Actually, you have to slow it down further. The original changes were clearly too fast, that's why users missed them and now need to re-inspect. By playing them back at 1/10 or 1/100 of the original speed, users will be able to catch up with what happened.

@georgweiss
Copy link
Collaborator

Sure, but for our use case "actual time" replay is essential. A multiplier < 1.0 would also make sense.

@shroffk shroffk merged commit beb4b39 into master Jun 27, 2024
2 checks passed
@shroffk
Copy link
Member Author

shroffk commented Jun 27, 2024

Sorry I accidentally merged this, I had planned to do this next week

@kasemir @katysaintin @tynanford
Your may products will have to be updated to include the archive reader module which has the implementations of the archive reader SPI's

#3062

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

Successfully merging this pull request may close these issues.

3 participants