jerryshao commented on code in PR #11977:
URL: https://github.com/apache/gravitino/pull/11977#discussion_r3568676939
##########
clients/client-python/build.gradle.kts:
##########
@@ -186,11 +186,21 @@ tasks {
args = listOf("scripts/generate_version.py")
}
- val integrationTest by registering(VenvTask::class) {
- doFirst {
+ val startGravitinoServer by registering {
+ doLast {
gravitinoServer("start")
}
+ }
+
+ val stopGravitinoServer by registering {
+ doLast {
+ gravitinoServer("stop")
+ }
+ }
+ val integrationTest by registering(VenvTask::class) {
+ dependsOn(startGravitinoServer)
+ finalizedBy(stopGravitinoServer)
Review Comment:
`finalizedBy` only runs if `integrationTest` itself is scheduled to execute.
If `startGravitinoServer` (a `dependsOn`) fails — e.g. the server never comes
up — `integrationTest` is skipped entirely and `stopGravitinoServer` never
fires, leaving a partially-started server process orphaned. I verified this
with a minimal Gradle repro (dependency task fails -> finalizer of the
dependent task never runs). Not a regression vs. the old single-task version
(same gap existed there), but it does mean this only covers *test* failures,
not *server-start* failures — might be worth a note in the description, or also
finalizing `startGravitinoServer` with the stop task.
##########
clients/client-python/tests/integration/integration_test_env.py:
##########
@@ -67,15 +67,18 @@ class IntegrationTestEnv(unittest.TestCase):
gravitino_startup_script = None
gravitino_admin_client: GravitinoAdminClient = None
+ @staticmethod
+ def use_external_gravitino() -> bool:
+ return os.environ.get("START_EXTERNAL_GRAVITINO", "").lower() == "true"
+
@classmethod
def setUpClass(cls):
- if (
- os.environ.get("START_EXTERNAL_GRAVITINO") is not None
- and os.environ.get("START_EXTERNAL_GRAVITINO").lower() == "true"
- ):
+ if cls.use_external_gravitino():
# Maybe Gravitino server already startup by Gradle test command or
developer manual startup.
if not check_gravitino_server_status():
- logger.error("ERROR: Can't find online Gravitino server!")
+ raise GravitinoRuntimeException(
Review Comment:
This changes behavior from logging-and-continuing to failing fast when an
external server isn't reachable. Looks like a deliberate improvement, but it's
a functional behavior change beyond the stated 'centralize the check' refactor
— might be worth calling out explicitly in the PR description.
--
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]
