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

Add MessageUpgrader to validator #286

Merged
merged 62 commits into from
Apr 5, 2022
Merged

Add MessageUpgrader to validator #286

merged 62 commits into from
Apr 5, 2022

Conversation

grafnu
Copy link
Collaborator

@grafnu grafnu commented Apr 1, 2022

No description provided.

@noursaidi
Copy link
Collaborator

The validator doesn't appear to be process state messages with this change - when a state message is published, nothing appears in the command line output or output directory.

I have compared against the current master (checksum 29530a7c8e265230d5127501feeee7b49811b1d5) and this does process state messages, so my message flow seems to be working. In both cases I was manually triggering state messages from an older instance of pubber so I'm certain there were state messages being sent

@grafnu
Copy link
Collaborator Author

grafnu commented Apr 4, 2022 via email

@noursaidi
Copy link
Collaborator

noursaidi commented Apr 4, 2022

I did deploy the cloud functions - I had to manually add the GCP project ID as an environment variable in the dashboard_deploy_gcloud script as it was coming up undefined (same issue as Francesco)

You have access to my GCP project. You run the validator with the following command
bin/validator <any site model> daq1-273309 udmi_target

I have configured a script to publish a static state payload every minute under device AHU-9 (you do have access to my site model but the payload I'm publishing doesn't match up with the site model)

@grafnu
Copy link
Collaborator Author

grafnu commented Apr 4, 2022 via email

@grafnu
Copy link
Collaborator Author

grafnu commented Apr 4, 2022 via email

@noursaidi
Copy link
Collaborator

I did add const PROJECT_ID = process.env.UDMI_GCP_PROJECT in the cloud function, I was testing something with Firebase deploy which is also not working for me.

Though for this, FIREBASE_CONFIG is set and within this there is a field projectId with the project ID which can be used in place of process.env.GCLOUD_PROJECT for this deploy method

It is now processing state messages and old payloads aren't triggering an error - but behaviour has changed. There used to be separate state_pointset.out and state_system.out etc, but now there's a single state.out - was this expected? It's also still not validating (against metadata) the points in pointset (issue #193)

@grafnu
Copy link
Collaborator Author

grafnu commented Apr 4, 2022 via email

@grafnu
Copy link
Collaborator Author

grafnu commented Apr 4, 2022 via email

@@ -379,7 +379,6 @@ private synchronized void cancelExecutor() {
if (scheduledFuture != null) {
try {
scheduledFuture.cancel(false);
scheduledFuture.get();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this creating problems or is it just cleanup to not wait? Sometimes these small changes are tricky to gauge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It actually causes problems -- once the Future is cancelled (previous line) the get() fails... Just never noticed it until I started to trace down errors...

@@ -473,7 +484,7 @@ private File makeDeviceDir(String deviceId) {
return deviceDir;
}

private String makeSchemaName(Map<String, String> attributes) {
private String mungeAndExtractSchema(Map<String, String> attributes) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It isn't immediately obvious why we are crossing subFolder and subType values.

Can we either make it more clear with comments, or, reference the doc? envelope.md doesn't explain it ... ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, you shamed me into fixing the underlying confusion, so the crossing the values is no longer required. It did involve some additional changes, but it's now a lot cleaner overall, I think.

@grafnu
Copy link
Collaborator Author

grafnu commented Apr 5, 2022

I'm submitting this now even though there might be further tweaks necessary, since it at least cleans up a bunch of underlying issues that might be impacting other threads. If there's any additional comments/changes, just post them and I can address in another PR.

@grafnu grafnu merged commit d573d5f into faucetsdn:master Apr 5, 2022
@grafnu grafnu deleted the validator branch April 5, 2022 15:20
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.

None yet

3 participants