[KYUUBI #2686] Fix lock bug if engine initialization timeout #2687

ulysses-you · 2022-05-18T06:02:19Z

Why are the changes needed?

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request

kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefSuite.scala

codecov-commenter · 2022-05-18T09:43:54Z

Codecov Report

Merging #2687 (11fad96) into master (8de2f5f) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

❗ Current head 11fad96 differs from pull request most recent head 1ba6538. Consider uploading reports for the commit 1ba6538 to get more accurate results

@@             Coverage Diff              @@
##             master    #2687      +/-   ##
============================================
- Coverage     64.32%   64.12%   -0.20%     
- Complexity       80      275     +195     
============================================
  Files           385      407      +22     
  Lines         18628    19105     +477     
  Branches       2523     2569      +46     
============================================
+ Hits          11983    12252     +269     
- Misses         5506     5703     +197     
- Partials       1139     1150      +11

Impacted Files	Coverage Δ
.../org/apache/kyuubi/ha/client/DiscoveryClient.scala	`56.52% <ø> (ø)`
...ha/client/zookeeper/ZookeeperDiscoveryClient.scala	`70.65% <100.00%> (+0.28%)`	⬆️
...cala/org/apache/kyuubi/metrics/MetricsSystem.scala	`84.09% <100.00%> (+0.36%)`	⬆️
...ain/scala/org/apache/kyuubi/engine/EngineRef.scala	`78.09% <100.00%> (+0.73%)`	⬆️
...n/spark/authz/ranger/FilteredShowObjectsExec.scala	`42.10% <0.00%> (-37.90%)`	⬇️
...park/authz/ranger/FilterDataSourceV2Strategy.scala	`57.14% <0.00%> (-22.86%)`	⬇️
...ache/kyuubi/plugin/spark/authz/OperationType.scala	`57.64% <0.00%> (-12.36%)`	⬇️
.../kyuubi/plugin/spark/authz/PrivilegesBuilder.scala	`68.84% <0.00%> (-11.09%)`	⬇️
...he/kyuubi/spark/connector/tpcds/TPCDSCatalog.scala	`35.29% <0.00%> (-6.82%)`	⬇️
...he/kyuubi/plugin/spark/authz/util/AuthZUtils.scala	`46.80% <0.00%> (-4.36%)`	⬇️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8de2f5f...1ba6538. Read the comment docs.

ulysses-you · 2022-05-18T10:48:25Z

unbelievable my local works well with the test code... but the ga failed sometime

ulysses-you · 2022-05-18T12:20:49Z

Seems the test is stable, cc @yaooqinn @turboFei @pan3793 @cfmcgrady

kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefSuite.scala

yaooqinn · 2022-05-18T13:05:48Z

what is the current behavior？

ulysses-you · 2022-05-18T13:11:04Z

what is the current behavior？

if three(more than two) request coming, if the first request initializes engine timeout, we will get two engines from the last two request.

yaooqinn · 2022-05-18T13:13:34Z

if A hangs with a big timeout， considering B/C are sequentially retries of A， so they are going to make server fe threads be exhausted？

yaooqinn · 2022-05-18T13:17:02Z

what is the current behavior？

if three(more than two) request coming, if the first request initializes engine timeout, we will get two engines from the last two request.

hmm？ so the timeout here means time to acquire the lock，not time to hold the lock？

ulysses-you · 2022-05-18T13:44:03Z

what is the current behavior？

if three(more than two) request coming, if the first request initializes engine timeout, we will get two engines from the last two request.

hmm？ so the timeout here means time to acquire the lock，not time to hold the lock？

yeah.. that's right

yaooqinn · 2022-05-18T14:01:36Z

so，shall we throw exception if timeout？

ulysses-you · 2022-05-19T01:31:35Z

For now, the timeout of ENGINE_INIT_TIMEOUT only considers the elapsed time of creating engine. That said, it does not consider the time of acquiring lock. I'm not sure if we should include the time when we are acquiring the lock. And if we want that, we should do some changes.

The current code path:

tryWithLock(discoveryClient) {
  val startTime ..
   if (started + timeout <= System.currentTimeMillis()) {
     throw .. 
  }
}

The expected code path:

val startTime ..
tryWithLockAndThrowWhenTimeout(startTime, discoveryClient) {
   if (started + timeout <= System.currentTimeMillis()) {
     throw .. 
  }
}

yaooqinn · 2022-05-19T06:09:08Z

kyuubi-ha/src/main/scala/org/apache/kyuubi/ha/client/zookeeper/ZookeeperDiscoveryClient.scala

+        //     to avoid client too long to waiting in concurrent.
+
+        // Return false means we are timeout
+        val acquired = lock.acquire(timeout, unit)


use TimeUnit.MILLISECONDS directly, the timeout is always millis

Can we keep the unit here? ETCD lease timeout is seconds, and cannot change unit.

simply timeout / 1000 for etcd?

yaooqinn · 2022-05-19T06:30:47Z

kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefSuite.scala

+
+    // we should only submit two engines, the last request should timeout and fail
+    assert(MetricsSystem.counterValue(ENGINE_TOTAL).get - beforeEngines == 2)
+    executor.shutdown()


shall we put it in the finally block?

yeah, added it

ulysses-you · 2022-05-19T08:26:46Z

thanks for review ! merging to master

ulysses-you · 2022-05-19T08:31:20Z

seems a lot of conflicts.. I will do backport manually

backport #2687 for branch-1.5 ### _Why are the changes needed?_ closes #2686 ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #2705 from ulysses-you/KYUUBI-2686-1.5. Closes #2686 8db5ee3 [ulysses-you] fix a8b17db [ulysses-you] engine type f76bfb7 [ulysses-you] Fix lock bug if engine initialization timeout Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: ulysses-you <ulyssesyou@apache.org>

backport apache#2687 for branch-1.5 ### _Why are the changes needed?_ closes apache#2686 ### _How was this patch tested?_ - [x] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes apache#2705 from ulysses-you/KYUUBI-2686-1.5. Closes apache#2686 8db5ee3 [ulysses-you] fix a8b17db [ulysses-you] engine type f76bfb7 [ulysses-you] Fix lock bug if engine initialization timeout Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: ulysses-you <ulyssesyou@apache.org>

github-actions bot added module:ha module:server labels May 18, 2022

turboFei reviewed May 18, 2022

View reviewed changes

kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefSuite.scala Outdated Show resolved Hide resolved

turboFei approved these changes May 18, 2022

View reviewed changes

kyuubi-server/src/test/scala/org/apache/kyuubi/engine/EngineRefSuite.scala Outdated Show resolved Hide resolved

Fix lock bug if engine initialization timeout

11fad96

ulysses-you force-pushed the lock branch from 01afa36 to 11fad96 Compare May 19, 2022 04:20

github-actions bot added the module:metrics label May 19, 2022

flaky test

1ba6538

yaooqinn reviewed May 19, 2022

View reviewed changes

hddong approved these changes May 19, 2022

View reviewed changes

address comment

868f95b

yaooqinn reviewed May 19, 2022

View reviewed changes

finally

e5dcacd

yaooqinn approved these changes May 19, 2022

View reviewed changes

ulysses-you closed this in c210fda May 19, 2022

ulysses-you deleted the lock branch May 19, 2022 08:31

ulysses-you mentioned this pull request May 19, 2022

[KYUUBI #2686][1.5] Fix lock bug if engine initialization timeout #2705

Closed

3 tasks

yaooqinn added this to the v1.6.0 milestone May 19, 2022

yaooqinn assigned ulysses-you May 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #2686] Fix lock bug if engine initialization timeout #2687

[KYUUBI #2686] Fix lock bug if engine initialization timeout #2687

ulysses-you commented May 18, 2022 •

edited

Loading

codecov-commenter commented May 18, 2022 •

edited

Loading

ulysses-you commented May 18, 2022

ulysses-you commented May 18, 2022

yaooqinn commented May 18, 2022

ulysses-you commented May 18, 2022

yaooqinn commented May 18, 2022

yaooqinn commented May 18, 2022

ulysses-you commented May 18, 2022

yaooqinn commented May 18, 2022

ulysses-you commented May 19, 2022

yaooqinn May 19, 2022

ulysses-you May 19, 2022

hddong May 19, 2022

yaooqinn May 19, 2022

hddong May 19, 2022

yaooqinn May 19, 2022

ulysses-you May 19, 2022

ulysses-you commented May 19, 2022 •

edited

Loading

ulysses-you commented May 19, 2022

[KYUUBI #2686] Fix lock bug if engine initialization timeout #2687

[KYUUBI #2686] Fix lock bug if engine initialization timeout #2687

Conversation

ulysses-you commented May 18, 2022 • edited Loading

Why are the changes needed?

How was this patch tested?

codecov-commenter commented May 18, 2022 • edited Loading

Codecov Report

ulysses-you commented May 18, 2022

ulysses-you commented May 18, 2022

yaooqinn commented May 18, 2022

ulysses-you commented May 18, 2022

yaooqinn commented May 18, 2022

yaooqinn commented May 18, 2022

ulysses-you commented May 18, 2022

yaooqinn commented May 18, 2022

ulysses-you commented May 19, 2022

yaooqinn May 19, 2022

Choose a reason for hiding this comment

ulysses-you May 19, 2022

Choose a reason for hiding this comment

hddong May 19, 2022

Choose a reason for hiding this comment

yaooqinn May 19, 2022

Choose a reason for hiding this comment

hddong May 19, 2022

Choose a reason for hiding this comment

yaooqinn May 19, 2022

Choose a reason for hiding this comment

ulysses-you May 19, 2022

Choose a reason for hiding this comment

ulysses-you commented May 19, 2022 • edited Loading

ulysses-you commented May 19, 2022

ulysses-you commented May 18, 2022 •

edited

Loading

codecov-commenter commented May 18, 2022 •

edited

Loading

ulysses-you commented May 19, 2022 •

edited

Loading