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

Fix resend historic data function #2946

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cvabc
Copy link
Contributor

@cvabc cvabc commented Dec 25, 2024

  • Backend's write function is implememted
  • RRD4j's floating number data is converted accordingly in Edge

Copy link

codecov bot commented Dec 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2946      +/-   ##
=============================================
+ Coverage      56.63%   56.64%   +0.01%     
- Complexity      9513     9518       +5     
=============================================
  Files           2253     2253              
  Lines          96074    96083       +9     
  Branches        7095     7096       +1     
=============================================
+ Hits           54401    54412      +11     
+ Misses         39666    39662       -4     
- Partials        2007     2009       +2     

Copy link
Contributor

@michaelgrill michaelgrill left a comment

Choose a reason for hiding this comment

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

I hope my comments are understandable, feel free to ask if they are not.

// return timestamps in milliseconds
resultMap.computeIfAbsent(timestamp * 1000, t -> new TreeMap<>()) //
.put(channelAddress, new JsonPrimitive(value));
.put(channelAddress, jval);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is just for reading resend data. Shouldnt this be also applied to e. g. local history request as well?
Maybe we should already save the values "correctly" = rounded?

Basically same as for sending to the backend:

var value = channel.getPastValues() //
.tailMap(channelStartTime, true) //
.entrySet() //
.stream() //
.filter(e -> e.getKey().isBefore(endTime.toLocalDateTime())) //
.filter(e -> e.getValue().isDefined()).map(e -> e.getValue().get()) //
.collect(aggregateCollector(channel.channelDoc().getUnit().isCumulated(), //
channel.getType()));
// TODO aggregation should be modifiable in Doc e. g. not every EnumDoc may want
// this behaviour
if (channel.channelDoc() instanceof EnumDoc) {
value = aggregateEnumChannel(channel, channelStartTime, endTime.toLocalDateTime());
}

this.writeData(//
edgeId, //
data, //
(influxEdgeId, channel) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are actually 2 reasons why we didnt implemented this one yet.

  1. (minor reason) If you have lots of edges online which are sending lots of data and your database is already overflowing with data you may not want to have extra load on your database

  2. Existing data could be overwritten with different data. If local data and remote (backend) data is not stored equally resending data could result in different data than before.
    Local Rrd4j stores data aggregated to every 5min with the timestamp of the starting timerange (data from 13:40 to< 13:45 would have the timestamp of 13:40)
    For cumulated values (energy) that would mean that if we lose connection lets say at 13:43 we have data until then and after resending the data our resend value at 13:40 could be greater than the values after that one because its actually the maximum value of 13:40-13:45. Then we could add 4min and 59seconds to make sure this value is at the end...
    For average values (power) after resending there could also be a data change because we add one more value to a timerange and therefore the average of that timerange could be different.

You can definitly add this but you should definitely have this in mind while using it.
I think the best would be to add an config option to enable/disable it with a doc with the disadvantages of it.

Also if you havent seen it thats why we actually use the AggregatedInflux it doesnt have those flaws but only stores defined data

data, //
(influxEdgeId, channel) -> {
this.timestampedChannelsForEdge.put(influxEdgeId, channel);
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

just return true and remove this.timestampedChannelsForEdge.put(influxEdgeId, channel); this is only relevant for "live" data

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.

2 participants