Allow watching resources over HTTP if watching using websocket fails #652

foxish · 2017-02-07T19:04:41Z

Falling back to using persistent HTTP Get.
The code could be DRYer. Getting this out there to get some initial opinions.

@iocanel @jimmidyson @iyanuobidele

foxish · 2017-02-07T19:14:53Z

Will fix tests shortly.

iocanel · 2017-02-07T22:03:25Z

ok to test

jimmidyson

Can we get a test in for this too? @iocanel Thoughts on using the mock test server for this?

jimmidyson · 2017-02-07T22:42:42Z

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java

      return watch;
    } catch (MalformedURLException e) {
      throw KubernetesClientException.launderThrowable(e);
+    } catch (KubernetesClientException ke) {


Should we check the type of the exception? For example, there would be no point in falling back to HTTP watch in e.g. auth error. Perhaps we should only fall back if we receive a 200 instead of a 101 for example?

Done, added a check for ke.getCode() to be 200.

jimmidyson · 2017-02-07T22:42:58Z

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java

+    } catch (KubernetesClientException ke) {
+      WatchHTTPManager watch = null;
+      try {
+        watch = new WatchHTTPManager(


Simply return new WatchHTTP....

jimmidyson · 2017-02-07T22:49:13Z

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/WatchHTTPManager.java

+      response = clonedClient.newCall(request).execute();
+      BufferedSource source = response.body().source();
+      while (!source.exhausted()) {
+        String message = source.readUtf8LineStrict();


Is each event new line separated then? Not chunked response? I haven't looked to be sure but will trust you :)

Yes, the chunk size is sent at first, but does not correspond to the event boundaries. The newlines are reliable separators.

jimmidyson · 2017-02-07T22:50:12Z

Thanks for this PR. It's looking good, I like the approach, just some minor questions and we need a test too if possible.

jimmidyson · 2017-02-07T22:50:52Z

Oh and this breaks some tests too - see build logs please.

foxish · 2017-02-08T02:04:37Z

@jimmidyson @iocanel PTAL. Addressed comments, added tests.

jimmidyson

LGTM! Thanks for the tests!

jimmidyson · 2017-02-08T09:17:05Z

When I try this locally, I get a 5 second delay between 200 response from websocket watcher request and the HTTP watcher kicking in:

09:15:48.719 [main] DEBUG io.fabric8.kubernetes.client.dsl.internal.WatchConnectionManager - Connecting websocket ... io.fabric8.kubernetes.client.dsl.internal.WatchConnectionManager@e5041ef
09:15:53.830 [OkHttp http://localhost:8001/...] WARN  io.fabric8.kubernetes.client.dsl.internal.WatchConnectionManager - Exec Failure: HTTP 200, Status: 200 -

Any ideas why?

iocanel · 2017-02-08T13:23:54Z

[merge]

jimmidyson · 2017-02-08T13:25:07Z

@iocanel I was waiting for feedback on #652 (comment)...

iocanel · 2017-02-08T13:27:13Z

@jimmidyson: you've approved the request though.

iocanel · 2017-02-08T13:29:35Z

I aborted the job on jenkins until you get feedback.

jimmidyson · 2017-02-08T13:40:38Z

@iocanel That is a good point. I had approved the code review, but was testing locally before merging. I will ensure from now on that review approval means a complete yes, not just code review approval. Sorry for the misunderstanding!

foxish · 2017-02-08T16:18:46Z

Thanks for the quick review. A 5 second delay seems quite substantial, and I don't remember running into this. I'll look into it shortly.

jimmidyson · 2017-02-08T16:35:49Z

@foxish Yeah it was unexpected, it could be environmental but I'm not sure how. Let me know if you have any ideas.

foxish · 2017-02-08T17:12:56Z

@jimmidyson It seems like the websocket call takes ~5 seconds to fail. The delay comes from the DEFAULT_WEBSOCKET_TIMEOUT which is set to 5 seconds.
Specifying it in the form: Config config = new ConfigBuilder().withMasterUrl("http://127.0.0.1:8001").withWebsocketTimeout(1000).build(); for example, brings the delay down to 1s.

jimmidyson · 2017-02-08T17:16:16Z

OK so sounds like we need to address the websocket timeout separately. Getting back a 200 should prevent the websocket from trying to connect and immediately return.

@foxish Would you have time to look at that fix? I'm happy to merge this in & get a fix in for that in a separate PR.

foxish · 2017-02-08T17:19:58Z

That sounds good. I'll send a follow-up PR for the fix. Would cutting a release be possible sometime early next week? We'd like to use these changes in our Spark integration efforts.

jimmidyson · 2017-02-08T17:29:45Z

We can do the release as soon as we have the fix for the delay.

jimmidyson · 2017-02-08T17:30:43Z

[merge] thanks @foxish!

foxish force-pushed the add-hanging-get branch 2 times, most recently from 8ac44d0 to bf34108 Compare February 7, 2017 19:06

Allow watching resources over HTTP if watching using websockets fails

00b78c7

foxish force-pushed the add-hanging-get branch from bf34108 to 00b78c7 Compare February 7, 2017 19:08

foxish mentioned this pull request Feb 7, 2017

spark-submit must work with the local kubectl proxy endpoint apache-spark-on-k8s/spark#63

Closed

iocanel requested review from iocanel and jimmidyson February 7, 2017 22:03

jimmidyson suggested changes Feb 7, 2017

View reviewed changes

foxish force-pushed the add-hanging-get branch 2 times, most recently from af183ee to 9262d47 Compare February 8, 2017 01:52

Added tests

edd59d5

foxish force-pushed the add-hanging-get branch from 9262d47 to edd59d5 Compare February 8, 2017 01:54

Remove unneeded import

17e350b

jimmidyson approved these changes Feb 8, 2017

View reviewed changes

iocanel approved these changes Feb 8, 2017

View reviewed changes

fusesource-ci merged commit daaba85 into fabric8io:master Feb 8, 2017

foxish deleted the add-hanging-get branch February 8, 2017 17:38

This was referenced Feb 8, 2017

Fix high latency when websocket connection fails #655

Merged

Bumping up kubernetes-client version to fix GKE and local proxy apache-spark-on-k8s/spark#105

Merged

foxish mentioned this pull request Dec 12, 2017

[SPARK-22648] [K8S] Spark on Kubernetes - Documentation apache/spark#19946

Closed

4 tasks

Allow watching resources over HTTP if watching using websocket fails #652

Allow watching resources over HTTP if watching using websocket fails #652

Uh oh!

Conversation

foxish commented Feb 7, 2017

Uh oh!

foxish commented Feb 7, 2017

Uh oh!

iocanel commented Feb 7, 2017

Uh oh!

jimmidyson left a comment

Choose a reason for hiding this comment

Uh oh!

jimmidyson Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

foxish Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

jimmidyson Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

jimmidyson Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

foxish Feb 8, 2017

Choose a reason for hiding this comment

Uh oh!

jimmidyson commented Feb 7, 2017

Uh oh!

jimmidyson commented Feb 7, 2017

Uh oh!

foxish commented Feb 8, 2017

Uh oh!

jimmidyson left a comment

Choose a reason for hiding this comment

Uh oh!

jimmidyson commented Feb 8, 2017

Uh oh!

iocanel commented Feb 8, 2017

Uh oh!

jimmidyson commented Feb 8, 2017

Uh oh!

iocanel commented Feb 8, 2017

Uh oh!

iocanel commented Feb 8, 2017

Uh oh!

jimmidyson commented Feb 8, 2017

Uh oh!

foxish commented Feb 8, 2017

Uh oh!

jimmidyson commented Feb 8, 2017

Uh oh!

foxish commented Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmidyson commented Feb 8, 2017

Uh oh!

foxish commented Feb 8, 2017

Uh oh!

jimmidyson commented Feb 8, 2017

Uh oh!

jimmidyson commented Feb 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

foxish commented Feb 8, 2017 •

edited

Loading