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

[rrd4j] Write asynchronously to database #14913

Merged
merged 4 commits into from
May 8, 2023
Merged

[rrd4j] Write asynchronously to database #14913

merged 4 commits into from
May 8, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented May 1, 2023

Related to #14888 #14900

This is completely untested.

@J-N-K J-N-K added bug An unexpected problem or unintended behavior of an add-on additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels May 1, 2023
lolodomo
lolodomo previously approved these changes May 2, 2023
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo
Copy link
Contributor

lolodomo commented May 2, 2023

I am using persistence (RRD4J) only for charts.
Of course, I could deploy this change but I don't know exactly what to do to test and validate the change.

@lolodomo
Copy link
Contributor

lolodomo commented May 5, 2023

Noone that could explain how I could test that ?
I don't know if it exists tools to consult RRD4J database.

@lolodomo
Copy link
Contributor

lolodomo commented May 5, 2023

Enabling DEBUG or TRACE logs should be probably a way to test that RRD4J is working as expected.

@lolodomo
Copy link
Contributor

lolodomo commented May 6, 2023

Tested by checking the DEBUG logs. Seems to work perfectly.
@J-N-K : please remove the draft status if this is ok for you and then I will merge the PR.

@J-N-K
Copy link
Member Author

J-N-K commented May 6, 2023

The only reason why I have/had doubts is the logic when an equal timestamp update is received.

@J-N-K J-N-K marked this pull request as ready for review May 6, 2023 11:20
@J-N-K J-N-K requested a review from a team as a code owner May 6, 2023 11:20
@lolodomo
Copy link
Contributor

lolodomo commented May 6, 2023

I see strange behaviour with chart rendering in BasicUI as if the chart was put in cache after its first rendering. Not sure it is related to this PR. I will come back to the official version to check if it is different.

@lolodomo
Copy link
Contributor

lolodomo commented May 6, 2023

It is apparently identical with the current version of the persistence service, so not related to your changes.
I am not sure if there is a problem or not. The first display of a certain chart is a little slow and then it is very fast as if the chart image was in cache. Strange, I have never noticed that until today.

@lolodomo
Copy link
Contributor

lolodomo commented May 6, 2023

The only reason why I have/had doubts is the logic when an equal timestamp update is received.

In fact, I can see that values are still stored in database when the item state is updated or every minute (I see the logs) but I have not checked if they are at the right time. I should probably check that with charts or maybe using the REST API to fetch persisted data.

@lolodomo
Copy link
Contributor

lolodomo commented May 6, 2023

It is apparently identical with the current version of the persistence service, so not related to your changes.
I am not sure if there is a problem or not. The first display of a certain chart is a little slow and then it is very fast as if the chart image was in cache. Strange, I have never noticed that until today.

I was using sitemap visibility to switch between different charts. I believe Basic UI is just hiding/showing a chart when playing with the visibility. If the chart was already loaded, it is of course fast to show it.
If I exit the page and enter again, I can see a delay to build the chart.
So there is probably nothing new here.

@lolodomo
Copy link
Contributor

lolodomo commented May 6, 2023

@J-N-K : in case of massive item state updates, it could lead to the creation of a massive number of threads. Is there no limit that we could reach quickly ? At least, I see we are using a dedicated thread pool.

Previously the method store was synchronized. Now the method internalStore is not synchronized, meaning concurrent updates of DB could now occur. Isn't it a potential problem ?

@J-N-K
Copy link
Member Author

J-N-K commented May 6, 2023

Maybe. Another solution would be to have a queue of entries for each database (similar to what I did in influxdb). I'm just not sure if we can do batch updates in RRD4J.

@lolodomo lolodomo dismissed their stale review May 6, 2023 20:06

Comments added after approval

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

When I send a command to a persisted item and then call the GET API with the default parameters, I do not get the value just persisted. It might be the sign of bad timestamps in database or the default ones generated by the REST API ?

10:54:07.795 [INFO ] [openhab.event.ItemCommandEvent       ] - Item 'EnConges' received command OFF
10:54:07.804 [INFO ] [openhab.event.ItemStateChangedEvent  ] - Item 'EnConges' changed from ON to OFF
10:54:07.825 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '1.0' in rrd4j database (again)
10:54:07.829 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '0.0' in rrd4j database

Request:
image
Result:
image
Is it due to your changes or not, I don't know but the result is unexpected.

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

If I restore the unchanged version of rrd4j, suddenly the REST API is showing again updated values.
So I would guess that the timestamps you use to store values are wrong (not in the last day) ?

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

So I would guess that the timestamps you use to store values are wrong (not in the last day) ?

But I don't see what could be wrong in your changed code...

@J-N-K
Copy link
Member Author

J-N-K commented May 7, 2023

I have added the timestamps to the log message, maybe it helps to figure out what is going on.

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

@J-N-K : to restore the previous behavior avoiding concurrent writings to database, you just need to move "synchronized" from method store to method internalStore.

That's being said, this synchronized also avoids concurrent writings to different rrd files while it should be possible. Only concurrent writings to the same database should be avoided. Probably something to enhance in a separate PR (no real idea how to implement that).

Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
Copy link
Member Author

J-N-K commented May 7, 2023

Probably we could add a per-name-lock for that.

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

With your last version:

19:43:47.119 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '1.0' with timestamp 1683481427 in rrd4j database (again)
19:43:47.124 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '0.0' with timestamp 1683481427 in rrd4j database
...
19:45:51.494 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '0.0' with timestamp 1683481551 in rrd4j database (again)
19:45:51.498 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '1.0' with timestamp 1683481551 in rrd4j database

And the result of API GET request:

...
    {
      "time": 1683450300000,
      "state": "ON"
    },
    {
      "time": 1683450300000,
      "state": "OFF"
    },
    {
      "time": 1683481500000,
      "state": "OFF"
    },
    {
      "time": 1683481500000,
      "state": "OFF"
    }
  ]
}

It looks like there is a factor 1000 between the logged timestamps and the time returned by REST API. Time 1683481427 and 1683481551 are not returned by the API.

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

It looks like there is a factor 1000 between the logged timestamps and the time returned by REST API. Time 1683481427 and 1683481551 are not returned by the API.

Maybe it is normal as the API field is a ZonedDateTime.
Maybe due to time zone conversion, the time value returned by the API will not be the same ?

I am going to erase my DB file to see if values are really written.

Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
Copy link
Member Author

J-N-K commented May 7, 2023

Could it be that the returned values are the boundaries of the bins in the RRD?

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

I tried again and I can confirm that values are stored in DB.

20:11:33.039 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '0.0' with timestamp 1683483093 in rrd4j database
...
20:12:31.161 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '0.0' with timestamp 1683483151 in rrd4j database (again)
20:12:31.164 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '1.0' with timestamp 1683483151 in rrd4j database
...

20:13:58.108 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '1.0' with timestamp 1683483238 in rrd4j database (again)
20:13:58.112 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '0.0' with timestamp 1683483238 in rrd4j database
...

20:16:53.055 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '0.0' with timestamp 1683483413 in rrd4j database (again)
20:16:53.057 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '1.0' with timestamp 1683483413 in rrd4j database

The result of REST API:

{
  "name": "EnConges",
  "datapoints": "11",
  "data": [
    {
      "time": 1683483060000,
      "state": "OFF"
    },
    {
      "time": 1683483120000,
      "state": "OFF"
    },
    {
      "time": 1683483120000,
      "state": "OFF"
    },
    {
      "time": 1683483180000,
      "state": "OFF"
    },
    {
      "time": 1683483180000,
      "state": "ON"
    },
    {
      "time": 1683483240000,
      "state": "ON"
    },
    {
      "time": 1683483240000,
      "state": "OFF"
    },
    {
      "time": 1683483300000,
      "state": "OFF"
    },
    {
      "time": 1683483300000,
      "state": "OFF"
    },
    {
      "time": 1683483360000,
      "state": "OFF"
    },
    {
      "time": 1683483360000,
      "state": "OFF"
    }
  ]
}

I would expect 7 values in DB but the API is returning 11 values !
I would have expected two values with a difference of 1 second but I got identical times.
I would have expected each time OFF/ON or ON/OFF but sometimes I got OFF/OFF.
The last value is wrong (OFF) while my current item state is ON !

All that looks very bad and I do not understand why. Let me try again the same scenario with the synchronous version.

@J-N-K
Copy link
Member Author

J-N-K commented May 7, 2023

Look at the timestamps: They are exactly 60s apart, which is the bin size of the RRD database. Remember: RRD collects the values in bins, depending on the consolidation function the values are average, max, min, first, last of the values in the bin. My guess would be that we see the first and last value of each bin in this case, which is why we have two values for each timestamp.

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

Look at the timestamps: They are exactly 60s apart, which is the bin size of the RRD database. Remember: RRD collects the values in bins, depending on the consolidation function the values are average, max, min, first, last of the values in the bin. My guess would be that we see the first and last value of each bin in this case, which is why we have two values for each timestamp.

I thought the two values were due we stored two value with oen second diff:

20:16:53.055 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '0.0' with timestamp 1683483413 in rrd4j database (again)
20:16:53.057 [DEBUG] [rd4j.internal.RRD4jPersistenceService] - Stored 'EnConges' as value '1.0' with timestamp 1683483413 in rrd4j database

Even with the unchanged version, the result of the API still looks strange to me (even if a little less), sometimes the last value does not match my current item state !!!

My current persistence strategy:

// persistence strategies have a name and a definition and are referred to in the "Items" section
Strategies {
        // for rrd charts, we need a cron strategy
        everyMinute : "0 * * * * ?"
}

Items {
        GTemperature*, GSuiviConsoElec* : strategy = everyChange, everyMinute, restoreOnStartup
        GVirtualData*, GParametres* : strategy = everyChange, restoreOnStartup
}

My item is in group GParametres, so I would expect to have a new value stored only when the item state is updated (everyChange).

I am really asking myself if the REST API and/or query method is working ! For me it is just unusable !

Is there another way to check the content of a rrd file ? Your code should work and I feel I am loosing my time testing it using the REST API !

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

Ok, maybe I am starting to understand. When adding a new value, in fact there is first a new value added for each minute before the old value and the new value, so it can be many values. When switching item from OFF to ON, this will generate OFF OFF OFF ... OFF ON
But I have still the feeling that sometimes the REST API does not return the very last value.

@lolodomo
Copy link
Contributor

lolodomo commented May 7, 2023

I will limit my tests to the rendering of charts.
Let's see how the charts evolve until tomorrow morning and if I find nothing strange, I will merge the PR.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Code LGTM, thank you

@lolodomo
Copy link
Contributor

lolodomo commented May 8, 2023

Ok, my charts in Main UI and Basic UI look good, and they include apparently the last value.

@lolodomo lolodomo merged commit 5fe99a1 into openhab:main May 8, 2023
@lolodomo lolodomo added this to the 4.0 milestone May 8, 2023
@J-N-K J-N-K deleted the rrd4j branch May 13, 2023 19:54
tb4jc pushed a commit to tb4jc/openhab-addons that referenced this pull request Jun 19, 2023
* [rrd4j] Write asynchronously to database
* add timestamps to log message

---------

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Thomas Burri <[email protected]>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Aug 9, 2023
* [rrd4j] Write asynchronously to database
* add timestamps to log message

---------

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Matt Myers <[email protected]>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
* [rrd4j] Write asynchronously to database
* add timestamps to log message

---------

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Jørgen Austvik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
additional testing preferred The change works for the pull request author. A test from someone else is preferred though. bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants