Fix pseudo-irregular time series direct reads#1781
Conversation
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
|
pseudo-irregular changes
tests
timingsFor this command localhost curl -sS -w '\nstatus=%{http_code} time=%{time_total}s bytes=%{size_download}\n' \
-H 'Accept: application/json;version=2' \
-H 'X-CWMS-LRTS-Formatting: true' \
'http://localhost:8081/cwms-data/timeseries?name=Baldhill_Dam.Flow-Out.Inst.~15Minutes.0.best&office=MVP&unit=cfs&begin=2025-05-01T00:00:00Z&end=2025-06-01T00:00:00Z&page-size=100000'national curl -sS -w '\nstatus=%{http_code} time=%{time_total}s bytes=%{size_download}\n' \
-H 'Accept: application/json;version=2' \
'https://cwms-data.usace.army.mil/cwms-data/timeseries?name=Baldhill_Dam.Flow-Out.Inst.~15Minutes.0.best&office=MVP&unit=cfs&begin=2025-05-01T00:00:00Z&end=2025-06-01T00:00:00Z&page-size=100000'(LRTS header not in Prod yet)
|
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
MikeNeilson
left a comment
There was a problem hiding this comment.
Note possible conflicts with #1780 depending on which gets merged in first.
|
TSV PR was merged in, conflicts need to be fixed. Also see my comments about LRTS methods and usage, feels like something is stuck down a rabbit hole of over thinking and needs to get dug out. Ask database if this TSID is LRTS or REGULAR interval |
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
3c4c715 to
f362ce4
Compare
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
f362ce4 to
cea2f38
Compare
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
MikeNeilson
left a comment
There was a problem hiding this comment.
Still some concerns with the overall logic, but some of that wasn't touched in this PR.
Actual concerns address in specific comments.
Otherwise, looks reasonable, and the tests show expected behavior correctly.
|
|
||
| Timestamp beginTimestamp = Timestamp.from(beginTime.toInstant()); | ||
| Timestamp endTimestamp = Timestamp.from(endTime.toInstant()); | ||
| String beginTimestampText = beginTimestamp.toLocalDateTime().format(ORACLE_DATE_FORMATTER); |
There was a problem hiding this comment.
why are we spending time formatting a timestamp that can be used as-is in the query?
There was a problem hiding this comment.
When I was trying to run the test for TimeSeriesDirectReadParityIT it started failing/saying missing.
I agree converting from one and back to the other is wasted cycles though. Any ideas on how to make it so it'll match up with AV_TSV_DQU.DATE_TIME?
There was a problem hiding this comment.
Hmm. that's, unexpected, especially since it's a begin/end time stamp so a few seconds on either side shouldn't matter much. Might have something to do with the columns being Date instead of Timestamp in the database.
Presumably the performance is still improved overall? If so, I'd argue that challenge is reasonable cause to leave the extra conversions in. Not like it happens for every row. Just put a comment in about why for future readers.
There was a problem hiding this comment.
Was it failing in your dev environment, or on GitHub Actions? I'm curious if your local time zone is getting in the mix somehow
There was a problem hiding this comment.
This was failing in my local integration test run. The GitHub checks were green with the current to_date(bindText) comparison.
When I switched the AV_TSV_DQU.DATE_TIME to direct Timestamp locally TimeSeriesDirectReadParityIT started returning missing/zero direct read rows compared to retrieve_ts. I left the request conversion and added a comment explaining. Probably onto something about the local TZ though.
There was a problem hiding this comment.
is your local database a docker instance or something else?
There was a problem hiding this comment.
Given the conversion only happens once, it's not a huge performance hit, some I'm okay with this explanation for the time being. We'll want to look into it more, but we don't think we should hold up this change just for that.
|
Spoke in CDA biweekly if RMA would also be able to review and ensure nothing was missed. |
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Signed-off-by: Charles Graham, SWT <charles.r.graham@usace.army.mil>
Summary
Fixes the time-series direct-read path for pseudo-irregular old-style local-regular IDs such as
*.~15Minutes.*whenX-CWMS-LRTS-Formatting: trueis present.Root cause
The direct-read path could resolve pseudo-irregular old-style IDs through new LRTS formatting, producing local-regular metadata and returning no values. It also treated
~intervals as regular without first honoringinterval_utc_offset = UTC_OFFSET_IRREGULAR, which could incorrectly invoke gap-fill behavior for pseudo-irregular data.Changes
~IDs whose old-format metadata has irregular interval offset.~15Minutesseries and verifies the CDA response matchesretrieve_tsunder the LRTS formatting header.Validation
./gradlew.bat :cwms-data-api:compileJava :cwms-data-api:compileTestJava --no-daemon./gradlew.bat :cwms-data-api:integrationTests --tests "*TimeSeriesDirectReadParityIT.pseudoIrregularReadWithLrtsHeaderMatchesRetrieveTs" "-PCDA_POOL_INIT_SIZE=5" "-PCDA_POOL_MAX_ACTIVE=10" "-PCDA_POOL_MAX_IDLE=5" "-PCDA_POOL_MIN_IDLE=2" --no-daemon./gradlew.bat :cwms-data-api:checkstyleMain :cwms-data-api:checkstyleTest --no-daemonNote: broader
TimeSeriesDirectReadParityITruns were not clean in this local container due fixture/setup issues before the relevant endpoint path was exercised, including Oracle seed partition check failures and a default-data county error.Closes #1745