-
Notifications
You must be signed in to change notification settings - Fork 14
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
TimeSeries retrieval broken on latest CWMS schema master branch #622
Comments
I wish it told you what the name of "specified field" was when it can't find it. |
@adamkorynta
Which the following:
It's the latest released CDA + the HEC T7 24.x.y schema database. |
I feel like something changed, either in jooq or the way we have it configured. We used to be able to pass null to those t/f args and the pl/sql default would get called. Now we have to pass a valid value. We saw that when finishing up the version work too. |
@rma-rripken or @adamkorynta Let me know if you want work on this, if not I need to do it, I think someone keeps pressuring Kevin to just randomly test CDA from CWMSVue/CAVI without regard to actually you know, checking to see if any updates have been made. |
I'm on it, at least for a day or two |
Do we know when this first started happening? I see there is a 24.04.05 schema installer on nexus. It seems like that would trigger the problem but it doesn't from what I can see so far. |
It was with 24.04.05 on the HEC T7, on an older CDA, it's entirely plausible we fixed it already and I just need to make a new release. So with |
I only started testing CDA against the mainline schema for the forecast updates, so I don't know any history beyond that. Good to know 24.04.05 isn't throwing the error though! |
I ran the TimeseriesControllerTestIT with registry.hecdev.net/cwms/schema_installer:latest-dev and then again with registry.hecdev.net/cwms/schema_installer:24.04.05. Neither had a problem so I'm not sure the issue has been fixed or if my test isn't in-depth enough. I'm going to look into that INVALID_T_F_FLAG error message. That is probably still present and if I can reproduce that then something isn't right in my env |
Interesting. This is the request that caused it (coming through hec-cwms-data-access, though I doubt that's the culprit:
Did just notice the "unit=SI" but I thought we already covered that case, and pretty sure hec-cwms-data-access always use the |
I'm able to duplicate the error. Even though you pointed it out I didn't appreciate the unit=SI thing until I was looking at my test case in more detail. I don't think that is the cause but I'm not sure if that should be an error or not. The parameter says it takes {EN, SI, Other} . I don't really know what Other does. Do users actually pass "Other" or is that suppose to mean that users can pass what they want? When I pass "SI" to the timeseries end-point I get back data in "in" so that part doesn't seem to be working. Was "Other" from legacy v1 pl/sql or is that a feature that is still supposed to work? |
Other would be the specific unit. So you can say: SI, EN , which would give you the default SI or EN units (for example meters or feet for elevation) or if today I decided I wanted a big number, I could say "given me this elevation in inches. CWMSVue (through hec-cwms-data-access) is doing the reasonable thing of just using the unit family, so it does need to work that way, but an individual query can arbitrarily choose there units, the database will throw an error if the conversion doesn't make any sense (like requesting CFS for an elevation.) |
The issue duplicates if the begin/end dates span a daylight savings switch over. That cryptic message "specified field not found in datetime or interval" is an Oracle message that I think means something is asking for a datetime that isn't valid. I need to dig a little more to figure out exactly where in our code that is happening. |
I never understood the 'other' bit either. The view (cwms_v_loc, etc.) has unit_system which is tied to an API call for default display units of the office signed in. The use case is to support output like Storage in kaf for offices like NWD-MR stem dams vs ac-ft for offices like NAE. Can the 'other' be just dropped ? Or is the underlying query tied to the dqu view and thus "other" could be a possible unit_id in there? |
I've made a new issue for updating the documentation and handling the EN/SI units part of this |
Its this query in CWMS_TS l_local_time_zone := cwms_loc.get_local_timezone(l_location_code); |
The pl/sql above is not the cause. With some help from @rma-psmorris I was able to dig into it a little deeper. I think my IDE had stale pl/sql from the previous schema. Once I refreshed things the error stopped pointing there. The error points at this line of code: https://bitbucket.hecdev.net/projects/CWMS/repos/cwms_database/browse/schema/src/cwms/cwms_ts_pkg_body.sql?at=refs%2Fheads%2Frelease%2F24.04.05#3556 "FETCH query_cursor INTO output_row" The cursor returned by retrieve_ts_out is the return value of a call to retrieve_ts_f.
|
Running this query in Intellij SQL Console doesn't reproduce the error but running it in SQL Developer does.
|
Setting sessiontimezone to UTC fixes the error.
|
I was about to say "ugh" but that actually makes sense. The database is in UTC and the host should be. But so should the session so maybe the connections aren't properly defaulting to such? |
What part of the schema changes to 24.04.05 made setting the sessiontimezone necessary? In that retreive_ts_f code I posted above we're in the "DATE" case btw. So the cursor is doing "nvl(q1.date_time, q2.date_time)" I haven't debugged into things further to figure out the exact types of q1.date_time or q2.date_time. Even though we pass "UTC" into retrieve_ts_f something isn't using it and is falling back on the sessiontimezone. Is this just a CDA thing? Or something specific to using the packaged Docker container? |
We could add some sql to the connection pool config that sets the sessiontimezone on the connection before a connection is loaned out from the pool. |
Quite possibly the LRTS work as Adam just found a test that files because of a missing location. But always setting the session TZ to a known value is probably is a good idea, OpenDCS behaves that say and it's not a slow call. |
Well, Mike P did just do a giant rewrite to that procedure for both LRTS and performance. |
@MikeNeilson Should I dig further into into zretrieve and try and find the bad cast? Or is this not a bug and just a mis-configuration? Must sessiontimezone always be utc? If it isn't then the other error messages aren't actually an error and I'm just using it wrong? |
Sessiontimezone should always be UTC, otherwise I think it messes with the date math we use. Did just that fix it or is there just another issue? (sorry, I think I mixed things up within this particular issue.) |
I have a test case that fails with that same error message. If I set sessiontimezone then the test case passes with no code changes. |
They I say we say it's the session and that code is now just part of the connection creation and handing out. |
#663 should have fixed this |
Trying to retrieve numerical time series results in the following error:
The text was updated successfully, but these errors were encountered: