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

Publisher Points Table #1482

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Publisher Points Table #1482

wants to merge 2 commits into from

Conversation

terrypacker
Copy link
Contributor

This is a proof of concept with a JUnit test for H2

The following fields are added to every point as they come for free with a AbstractActionVO:

id
xid
name
enabled	

Do we want to add:

readPermission
setPermission

NOTES:

  1. The ‘data’ column stores JSON instead of binary data. I used a CLOB type like the Advanced Scheduler. One point of interest is how to do versioning with module based extension of a PublishedPointVO, I took a crack at it but would like someone to review this.

  2. The upgrade code does not exist yet, the plan is to set all points to enabled and the name to be the data point name.

  3. The legacy JSON for a publisher will need to be supported in addition to a new export of publisherPoints. This will work in the PublisherImporter by checking for the ‘points’ JSON and then importing them separately. i.e. the publisher’s jsonRead method will ignore the ‘points’ JSON and this will be handled in the importer logic.

  4. I will likely add generics to the PublishedPointDao

  5. It is implied that you can save each point individually now, so we will need to modify the RuntimeManager and Publisher RT to support start/stop/restart of published points. But if we don't expose this via REST we can push this off till later if desired.

@terrypacker terrypacker added this to the 3.7.0 Release milestone Aug 30, 2019
@terrypacker terrypacker requested review from jazdw and Puckfist August 30, 2019 17:49
@terrypacker
Copy link
Contributor Author

terrypacker commented Aug 30, 2019

This fixes #1468

@jazdw
Copy link
Contributor

jazdw commented Sep 3, 2019

I think there is zero purpose to having a name property for every published point.
I am also skeptical that each published point should have an XID or ID. Basically I am not sure they should be treated as independent objects rather than properties of a publisher.

Re. readPermission, setPermission and enabled. These are new features that are unnecessary IMO. Lets keep it simple.

The ‘data’ column stores JSON instead of binary data. I used a CLOB type like the Advanced Scheduler. One point of interest is how to do versioning with module based extension of a PublishedPointVO, I took a crack at it but would like someone to review this.

I think it is a good idea to use JSON instead of Java serialized binary data. We definitely should store a version property, however I am not willing to dig right into this right now. I think another area we should look at is using a JSON data type column where supported, e.g. MySQL

The legacy JSON for a publisher will need to be supported in addition to a new export of publisherPoints. This will work in the PublisherImporter by checking for the ‘points’ JSON and then importing them separately. i.e. the publisher’s jsonRead method will ignore the ‘points’ JSON and this will be handled in the importer logic.

Again, not sure we should import/export them separately like this.

It is implied that you can save each point individually now, so we will need to modify the RuntimeManager and Publisher RT to support start/stop/restart of published points. But if we don't expose this via REST we can push this off till later if desired.

We should definitely allow adding/removing published points from a publisher without restarting it.

@jazdw
Copy link
Contributor

jazdw commented Sep 3, 2019

This fixes #1478

Pretty sure you linked the wrong issue here?

@terrypacker
Copy link
Contributor Author

@jazdw yes, wrong issue. I updated the comment to refer to the correct issue.

@terrypacker
Copy link
Contributor Author

@jazdw

I think there is zero purpose to having a name property for every published point.
I am also skeptical that each published point should have an XID or ID. Basically I am not sure they should be treated as independent objects rather than properties of a publisher.

I agree about the name.

The id would be used like a data point id, if one wanted to update a single published point. I'm open to discussion on this.

The xid would be used during import to update the configuration for a published point that already exists. Consider a ModbushPublishedPointVO that has settings beyond what is in the base class.

Since we want to be able to add/remove points from a running publisher without restarting it I assumed we would also want to be able to able to modify a running published point. This implies that one could use the JSON Import feature to do this, but I guess this isn't necessary.

Before going any further I think we should discuss what we are trying to achieve with this change. Specifically if we need anything other than a nice way to query publisher points and add/remove them from a publisher while it is running without restarting it. I'd also like to know the types of queries we will be doing from the UI as I suspect we will want to join on the dataPoints table to be able to query the source point's settings.

@terrypacker
Copy link
Contributor Author

Upon further investigation I think we can get away with extending AbstractBasicVO which only requires an id field. This will give us access to the AbstractBasicDao that allows RQL and JOOQ conditions.

Many things outstanding, this is just a proof of concept for H2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants