spark-test commented on code in PR #59695:
URL: https://github.com/apache/spark/pull/59695#discussion_r4348080573
##########
dev/merge_spark_pr.py:
##########
@@ -2205,19 +2224,53 @@ def get_current_ref():
return ref
+def jira_is_live():
Review Comment:
**Nit (P3):** None of the new disabled-mode logic is covered by the script's
inline doctests: `jira_is_live()`'s classification, the `initialize_jira()`
disabled branch, the `jira_disabled` early return in `update_jira_for_pr()`,
and the `jira_ids` clearing in `main()`. CI ignores this file
(`dev/sparktestsupport/modules.py:56`), so `doctest.testmod()` is its only
automated check, and the existing `__test__` cases all run with the default
`jira_disabled=False`.
As a result, changing `e.code < 500` to `e.code < 400`, or deleting the
`update_jira_for_pr` guard or moving it below the MINOR/TRIVIAL/FOLLOWUP skip,
would pass every doctest. The first sign would be a live JIRA silently
disabled, or an `AttributeError` on `asf_jira=None` after the merge commit is
already pushed. The existing `patch.dict(update_jira_for_pr.__globals__, ...)`
pattern makes these cheap to cover offline: patch `urlopen` for a normal
response, `HTTPError` 404 and 503, and `URLError`/`socket.timeout`, and run
`update_jira_for_pr` with `jira_disabled=True` for revert and non-revert
titles, asserting that the reverter, resolver and confirmation mocks are never
called.
**Verification:**
- **Regression:** Changing the 5xx boundary of the probe classification (for
example to below 400) makes a doctest fail.
- **Regression:** Removing the jira_disabled guard from update_jira_for_pr,
or moving it after the MINOR/TRIVIAL/FOLLOWUP skip, makes a doctest fail.
- **Compatibility:** All existing merge-script doctests still pass alongside
the new ones, with jira_disabled left at its default of False for them.
##########
dev/merge_spark_pr.py:
##########
@@ -2205,19 +2224,53 @@ def get_current_ref():
return ref
+def jira_is_live():
+ """Quick liveness probe for the SPARK JIRA backend, to decide whether to
touch JIRA at all.
+
+ Returns False -- so the merge proceeds with JIRA disabled rather than
failing or hanging --
+ when JIRA is down, errors out, or takes longer than JIRA_READ_TIMEOUT to
answer. The ASF JIRA
+ instance goes down for infra maintenance often enough to be worth
tolerating.
+ """
+ # issues.apache.org is a reverse proxy in front of several backends, so
the front door can be
+ # up while the backend serving SPARK returns 503. Probe a SPARK-specific
endpoint so the result
+ # reflects whether SPARK operations will actually work. A 4xx still means
a backend answered
+ # (up); a 5xx (e.g. that 503) or a connection/timeout error means it is
effectively dead.
+ url = "%s/rest/api/2/project/SPARK" % JIRA_API_BASE
+ try:
+ with urlopen(Request(url), timeout=JIRA_READ_TIMEOUT):
+ return True
+ except HTTPError as e:
+ return e.code < 500
+ except Exception:
+ return False
+
+
def initialize_jira():
- # JIRA access is mandatory: a successful merge must resolve the associated
ticket. Bail out
- # early -- before any git work -- if a prerequisite is missing, rather
than silently merging
- # and leaving the JIRA open (which is easy to miss and tedious to
reconcile after the fact).
- global asf_jira
+ # A successful merge should resolve its ticket, so JIRA access is normally
mandatory and we
+ # bail out early -- before any git work -- on a missing library or
credentials. But the ASF
+ # JIRA instance is not always up; probe it first and, if it is unreachable
or too slow, disable
+ # JIRA for this run (asf_jira stays None) and let the merge proceed,
reconciling by hand later.
+ global asf_jira, jira_disabled
asf_jira = None
+ jira_disabled = False
jira_server = {"server": JIRA_API_BASE}
+ if not jira_is_live():
+ jira_disabled = True
+ print_error(
+ "JIRA is unreachable (%s did not answer within %gs); disabling all
JIRA steps for "
Review Comment:
**Nit (P3):** This message always says JIRA `did not answer within %gs`, but
`jira_is_live()` also returns False when the server *did* answer, with a 5xx
(`return e.code < 500`), and for any non-HTTP exception (DNS failure,
connection refused, `ssl.SSLError`, proxy errors) via the bare `except
Exception`. urllib raises `HTTPError` as soon as a non-2xx response arrives, so
the 503 that the probe's comment names as the main motivation is reported here
as a 30s timeout.
In disabled mode this line is the committer's only cue. A misattributed
cause can lead them to raise `JIRA_READ_TIMEOUT`, which cannot help with a 503
or a TLS error, or to take a local CA/proxy problem (which used to fail fast)
for an ASF outage and merge with JIRA steps skipped. Have the probe return or
print the HTTP status or exception it hit, and name that cause here, or use
neutral wording such as "is unreachable or unhealthy".
**Verification:**
- **Behavior:** With urlopen stubbed to raise HTTPError 503, the printed
disable message names the 503 response and does not say JIRA failed to answer
within the timeout.
- **Behavior:** With urlopen stubbed to raise a connection or TLS error and
then a socket timeout, the printed message identifies each error, and JIRA is
still disabled in both cases.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
