Release/1.16.0 #101

it-hms · 2024-08-15T13:50:41Z

This PR addresses the issue of time zone handling

support maintaining JVM time zone offset from GMT/UTC. This is needed for many java features to work properly. Previous the time zone offset was only calculated once at start. Now it is possible to continuously check and update offset
Change EBD historical data calls to use relative format and not absolute. Absolute format utilized local timestamps; there is inherit limitations of this type of system

Additional feature of Instant values EBD call to return instant tag values

closes #98
closes #90

This change sets the simpleDateFormat (for local time parsing) to have the GMT time zone. Move GMT timezone name to TimeLibConstants and make public. This is necessary because the function finds the difference between system time (epoch millisecond relative to GMT) to local time ( epoch millisecond relative to GMT). Before setting the default time zone for the JVM, this explicit setting is not necessary. However, after setting the default time zone, the simpleDateFormat will use the default time zone, which is often not GMT. This causes the time difference to be incorrect.

alexjhawk

It looks like you're using the branch release/1.16.0. The release/* branches should not be touched by any developer, including all of us, as they are intended to be created automatically by our automation when a tagged release is created.

See GitHub Actions Workflow here: https://github.com/hms-networks/sc-ewon-flexy-extensions-lib/blob/main/.github/workflows/release-make-branch.yml

I'm honestly not sure whether any issues will arise if you merge this PR and tag a release, but in practice, it's just best to avoid using the branch namespace of release/* entirely. I personally tend to name development branches in the format of dev/1.2.3 or a number related to a related GitHub issue.
- As long as you enable the 'Delete Branch' option when merging this PR, the automation will probably run fine, though.

alexjhawk · 2024-08-15T14:07:13Z

...main/java/com/hms_networks/americas/sc/extensions/system/time/LocalTimeOffsetCalculator.java

+   */
+  public static long convertLocalEpochMillisToUtc(long localEpochMillis)
+      throws InterruptedException, ParseException {
+    return localEpochMillis + calculateLocalTimeOffsetMilliseconds();


Should this use getLocalTimeOffsetMilliseconds() instead?

In its current form, this method would force a local time offset calculation each time it is invoked. If it's being invoked for every historical data EBD response, that can be very expensive performance-wise, and it is unnecessary to update the time offset at such a fast rate (potentially many times a minute).

getLocalTimeOffsetMilliseconds() uses the last calculated offset - this could be outdated. Here we need a fresh calculation because the offset could have changed due to daylight savings or user setting

This still seems like a costly operation to perform each time. The name/Javadocs/appearance of the method also does not convey that it is updating the local time offset calculation. Based on the method name and Javadocs, I would expect that this method is just simply converting local epoch millis to UTC.

I understand the intention here, but I wonder whether there is a solution that does not put even further pressure on the already constrained Flexy by adding more EBD calls.

Additionally, this is also circumventing the SCTimeUtils class process for injecting time zone configurations, as well as the newly created TimeZoneManager class you're adding. It might make sense to align with those classes to ensure that if there is updated time offset information being pulled, we're also accordingly updating the JVM TZ configuration, too.

I think you are correct the TimeZoneManager class, if used, should maintain the offset.
Right now this method is only called when Rapid Catch up finds data and data point timestamps are local.
I will make the change.

src/main/java/com/hms_networks/americas/sc/extensions/historicaldata/RapidCatchUp.java

src/main/java/com/hms_networks/americas/sc/extensions/historicaldata/HistoricalDataManager.java

.../java/com/hms_networks/americas/sc/extensions/historicaldata/HistoricalDataQueueManager.java

...in/java/com/hms_networks/americas/sc/extensions/historicaldata/HistoricalDataEBDRequest.java

TomKimsey

A lot of instances of comments that start with lowercase. I dont recall if this was ever documented but i think it would be more consistent with the rest of the library if they were capitalized.

web-docs/docs/02-CHANGELOG.mdx

src/main/java/com/hms_networks/americas/sc/extensions/system/time/TimeZoneManager.java

...in/java/com/hms_networks/americas/sc/extensions/historicaldata/HistoricalDataEbdRequest.java

alexjhawk

Looks good aside from Google Java Format naming violations.

...in/java/com/hms_networks/americas/sc/extensions/historicaldata/HistoricalDataEbdRequest.java

This class is used to create a request for historical data EBD requests.

This method changes locally relative epoch time to a epoch time that is relative to UTC.

Updates include a check to ensure that the RapidCatchUp never looks for data past the current time. When RapidCatchUp finds data in the historical data queue, it will now convert the time to milliseconds since epoch UTC. This requires an understanding of the "Record Data in UTC" setting.

With EBD request building consolidated to HistoricalDataEBDRequest, methods were added that accept the EBD string as a parameter.

Refactor the method getFifoNextSpanDataRaw with the goals of readability and simplicity. Additionally, make use of methods to build EBD requests.

it-hms requested review from alexjhawk and TomKimsey August 15, 2024 13:56

alexjhawk requested changes Aug 15, 2024

View reviewed changes

it-hms force-pushed the release/1.16.0 branch 3 times, most recently from aa440da to fad8948 Compare August 15, 2024 17:39

TomKimsey requested changes Aug 16, 2024

View reviewed changes

Added TimeZoneManager

1187f1b

it-hms force-pushed the release/1.16.0 branch from fad8948 to 28407e8 Compare August 20, 2024 15:20

it-hms requested review from alexjhawk and TomKimsey August 20, 2024 15:23

TomKimsey previously approved these changes Aug 20, 2024

View reviewed changes

alexjhawk requested changes Aug 20, 2024

View reviewed changes

...in/java/com/hms_networks/americas/sc/extensions/historicaldata/HistoricalDataEbdRequest.java Outdated Show resolved Hide resolved

it-hms added 2 commits August 20, 2024 12:51

Add HistoricalDataEbdRequest class

ba2e464

This class is used to create a request for historical data EBD requests.

Add convertLocalEpochToUTC

90875f1

This method changes locally relative epoch time to a epoch time that is relative to UTC.

it-hms dismissed TomKimsey’s stale review via 400c21e August 20, 2024 16:51

it-hms force-pushed the release/1.16.0 branch from 28407e8 to 400c21e Compare August 20, 2024 16:51

it-hms added 4 commits August 20, 2024 12:55

HistoricalDataManager no longer creates EBD req

e8dbc8f

With EBD request building consolidated to HistoricalDataEBDRequest, methods were added that accept the EBD string as a parameter.

Update HistoricalDataQueueManager

8dd20f4

Refactor the method getFifoNextSpanDataRaw with the goals of readability and simplicity. Additionally, make use of methods to build EBD requests.

RELEASE: v1.16.0

a71fe6e

it-hms force-pushed the release/1.16.0 branch from 400c21e to a71fe6e Compare August 20, 2024 16:56

it-hms requested review from alexjhawk and TomKimsey August 20, 2024 17:24

TomKimsey approved these changes Aug 20, 2024

View reviewed changes

alexjhawk approved these changes Aug 20, 2024

View reviewed changes

it-hms merged commit d3394dd into main Aug 21, 2024
5 checks passed

it-hms deleted the release/1.16.0 branch August 21, 2024 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release/1.16.0 #101

Release/1.16.0 #101

it-hms commented Aug 15, 2024 •

edited

Loading

alexjhawk left a comment

alexjhawk Aug 15, 2024

it-hms Aug 15, 2024

alexjhawk Aug 15, 2024

it-hms Aug 15, 2024

TomKimsey left a comment

alexjhawk left a comment

Release/1.16.0 #101

Release/1.16.0 #101

Conversation

it-hms commented Aug 15, 2024 • edited Loading

alexjhawk left a comment

Choose a reason for hiding this comment

alexjhawk Aug 15, 2024

Choose a reason for hiding this comment

it-hms Aug 15, 2024

Choose a reason for hiding this comment

alexjhawk Aug 15, 2024

Choose a reason for hiding this comment

it-hms Aug 15, 2024

Choose a reason for hiding this comment

TomKimsey left a comment

Choose a reason for hiding this comment

alexjhawk left a comment

Choose a reason for hiding this comment

it-hms commented Aug 15, 2024 •

edited

Loading