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

Point value cache changes #1431

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

Point value cache changes #1431

wants to merge 10 commits into from

Conversation

terrypacker
Copy link
Contributor

Improvements to PointValueCache that make it thread safe for query and insert operations. Also the cache will now retain values in "Point Value Purgatory" to make them available for queries on the DataPointRT via a callback on point value insertion to the DB.

Note I think there is still a small time window after the callback where the value may not be available in the database due how the database handles the batch insert.

There are 2 tests that attempt to prove the solution:

com.serotonin.m2m2.rt.dataImage.PointValueCacheTest
com.serotonin.m2m2.rt.dataImage.DataPointRTQueriesTest

Related issues:

#1428
#1427

@terrypacker terrypacker requested review from jazdw and Puckfist May 16, 2019 19:01
@terrypacker terrypacker force-pushed the point-value-cache-changes branch from 045d90a to 334e42e Compare May 17, 2019 19:39
@Puckfist
Copy link
Contributor

It occurs to me on this branch that we may wish to change the functions in AbstractPointWrapper extenders to default to using the cache on previous and past functions.

@Puckfist
Copy link
Contributor

Looking at PointValueCache right now, it seems to have some curious behaviors around expanding maxSize

My impression was that,

  1. All values that are to be logged should be cached until database says logged and they're bumped out by a new value (to keep the cache of sampled values at the configured size)
  2. Caches should not expand beyond their configured size to retain interval sampled information (still has a TODO)
  3. Caches should not expand based on previous n value queries.

Which leads me to believe maxSize and defaultSize could be combined to configuredSize or just size and made final.

@terrypacker terrypacker force-pushed the point-value-cache-changes branch from 2960cb4 to 3100539 Compare June 24, 2019 23:34
@jazdw
Copy link
Contributor

jazdw commented Sep 3, 2019

Yeah I'm going to need more background information on what problem you are trying to solve. Along with a summary of implications of these changes.

@Puckfist
Copy link
Contributor

The problem to be solved is that there is caching and buffering by multiple mechanisms as points are gathering data. The NoSQL database has a buffer for making batch writes, for instance. If something immediately attempts to access the data that was collected, such as a meta point, it is possible (likely if not using cache, as became the default some versions ago for scripting AbstractPointWrapper implementors) that the data won't be available to it.

The proposed solution is a modification of the PointValueCache all DataPointRT objects have which retains all values until there is a callback from the database that the value has been written. This would make queries run on the values from event driven uses, such as meta points or event handlers, return all the values that generated the events (think for instance of a data file data source importing a day's worth of data and generating events all at once).

The only negative implication in the implementation in the PR is that all interval logging period samples are retained in the cache where perhaps they would not be retained in memory if the interval logging type was maximum, minimum or instant. All logged values are retained in memory until written to disk, so that is not a memory add. It would also do away with the old behavior of the cache expanding with calls to the getLatestPointValues function in the PointValueCache (which shrinks again during the nightly purge), because caches will always be their default cache size or larger if necessary to track values not yet written.

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