rzo1 commented on code in PR #169:
URL: https://github.com/apache/openjpa/pull/169#discussion_r3897688386
##########
openjpa-persistence/src/main/java/org/apache/openjpa/persistence/QueryImpl.java:
##########
@@ -780,20 +780,28 @@ public TypedQuery<X> setCacheStoreMode(CacheStoreMode
cacheStoreMode) {
@Override
public Integer getTimeout() {
- Object val = getHints().get(JPAProperties.QUERY_TIMEOUT);
- if (val instanceof Integer) {
- return (Integer) val;
- }
- if (val instanceof Number) {
- return ((Number) val).intValue();
- }
- return null;
+ int timeout = getFetchPlan().getQueryTimeout();
Review Comment:
Good catch, it was not OK. The write path is fine — the value reliably
reaches the plan, and the JPA key is the highest-ranked one so the precedence
check can't swallow it — but deriving null from `timeout > 0` was wrong both
ways: an explicit `setTimeout(0)` read back as null, and a query that only
*inherited* a timeout reported one that was never set on it. `getTimeout()` now
returns null when nothing is set on the query and the effective value
otherwise, so both sides answer from the same source.
Chasing this also turned up a side effect of the old `setTimeout(null)`: it
re-recorded the inherited value as an explicit
`jakarta.persistence.query.timeout` hint, which made the highest-ranked key
"set" and silently turned a later `setHint("openjpa.FetchPlan.QueryTimeout",
n)` into a no-op. It now drops the hint instead.
##########
openjpa-persistence/src/main/java/org/apache/openjpa/persistence/QueryImpl.java:
##########
@@ -780,20 +780,28 @@ public TypedQuery<X> setCacheStoreMode(CacheStoreMode
cacheStoreMode) {
@Override
public Integer getTimeout() {
- Object val = getHints().get(JPAProperties.QUERY_TIMEOUT);
- if (val instanceof Integer) {
- return (Integer) val;
- }
- if (val instanceof Number) {
- return ((Number) val).intValue();
- }
- return null;
+ int timeout = getFetchPlan().getQueryTimeout();
+ return timeout > 0 ? timeout : null;
}
+ /**
+ * Sets the query timeout in milliseconds. A null timeout clears a
timeout
Review Comment:
It wasn't, on two counts, and it's rewritten. `setTimeout(null)` clears the
timeout whatever set it — `setHint("jakarta.persistence.query.timeout", n)` or
`getFetchPlan().setQueryTimeout(n)` just as much as `setTimeout(n)` — and it
restores the value the entity manager carries *at that point*, not the one the
query was created with: the query's fetch config is cloned from the broker at
creation, while `inheritedQueryTimeout()` reads the EM live.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
