Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-915

Gremlin Server supports REST and Websockets simultanteously

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Done
    • Affects Version/s: 3.0.2-incubating
    • Fix Version/s: 3.2.6
    • Component/s: server
    • Labels:
      None

      Description

      Develop a Channelizer that allows REST and Websockets to be configured at the same time. I've personally tried to do this on a couple of attempts while following a Netty sample, but I've never been able to get it to work. Perhaps folks like Jason Plurad or Dylan Millikin would like to give it a go some day?

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user krlohnes opened a pull request:

          https://github.com/apache/tinkerpop/pull/618

          TINKERPOP-915Add combined handler for Http and Websockets

          TINKERPOP-915(https://issues.apache.org/jira/browse/TINKERPOP-915)

          Most of this is tests. I added an integration test that goes over the
          functionality of both the http and websocket channelizers using the new
          `WsAndHttpChannelizer`. I added an additional test on top of that to
          switch between using WebSockets and http.

          The change itself leverages the existing `WebSocketChannelizer` to
          provide the base pipeline setup. It has everything needed handler-wise
          to service both http and ws connections. The
          `WsAndHttpChannelizerHandler` then detects whether the incoming request
          is a plain http connection or a WebSockets connection. If it's an http
          connection, the channelizer handler swaps out the request handler
          appropriately for whether or not authentication has been enabled.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/krlohnes/tinkerpop add_combined_http_ws_handler

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/618.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #618


          commit 928d9a6566a4387fd91f43425f8a6ff77a7bce66
          Author: Keith Lohnes <krlohnes@us.ibm.com>
          Date: 2017-05-30T14:02:54Z

          TINKERPOP-915Add combined handler for Http and Websockets

          TINKERPOP-915(https://issues.apache.org/jira/browse/TINKERPOP-915)

          Most of this is tests. I added an integration test that goes over the
          functionality of both the http and websocket channelizers using the new
          `WsAndHttpChannelizer`. I added an additional test on top of that to
          switch between using WebSockets and http.

          The change itself leverages the existing `WebSocketChannelizer` to
          provide the base pipeline setup. It has everything needed handler-wise
          to service both http and ws connections. The
          `WsAndHttpChannelizerHandler` then detects whether the incoming request
          is a plain http connection or a WebSockets connection. If it's an http
          connection, the channelizer handler swaps out the request handler
          appropriately for whether or not authentication has been enabled.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user krlohnes opened a pull request: https://github.com/apache/tinkerpop/pull/618 TINKERPOP-915 Add combined handler for Http and Websockets TINKERPOP-915 ( https://issues.apache.org/jira/browse/TINKERPOP-915 ) Most of this is tests. I added an integration test that goes over the functionality of both the http and websocket channelizers using the new `WsAndHttpChannelizer`. I added an additional test on top of that to switch between using WebSockets and http. The change itself leverages the existing `WebSocketChannelizer` to provide the base pipeline setup. It has everything needed handler-wise to service both http and ws connections. The `WsAndHttpChannelizerHandler` then detects whether the incoming request is a plain http connection or a WebSockets connection. If it's an http connection, the channelizer handler swaps out the request handler appropriately for whether or not authentication has been enabled. You can merge this pull request into a Git repository by running: $ git pull https://github.com/krlohnes/tinkerpop add_combined_http_ws_handler Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/618.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #618 commit 928d9a6566a4387fd91f43425f8a6ff77a7bce66 Author: Keith Lohnes <krlohnes@us.ibm.com> Date: 2017-05-30T14:02:54Z TINKERPOP-915 Add combined handler for Http and Websockets TINKERPOP-915 ( https://issues.apache.org/jira/browse/TINKERPOP-915 ) Most of this is tests. I added an integration test that goes over the functionality of both the http and websocket channelizers using the new `WsAndHttpChannelizer`. I added an additional test on top of that to switch between using WebSockets and http. The change itself leverages the existing `WebSocketChannelizer` to provide the base pipeline setup. It has everything needed handler-wise to service both http and ws connections. The `WsAndHttpChannelizerHandler` then detects whether the incoming request is a plain http connection or a WebSockets connection. If it's an http connection, the channelizer handler swaps out the request handler appropriately for whether or not authentication has been enabled.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          This is really great stuff. I tried doing this a few times and for whatever reason I couldn't get it to work right. Looking forward to reviewing this one.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 This is really great stuff. I tried doing this a few times and for whatever reason I couldn't get it to work right. Looking forward to reviewing this one.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          I probably should have mentioned this on my last post, but I probably won't review this until next week as we are currently preparing for release and the release branches are frozen during testing.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 I probably should have mentioned this on my last post, but I probably won't review this until next week as we are currently preparing for release and the release branches are frozen during testing.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @spmallette Thanks for the heads up.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 @spmallette Thanks for the heads up.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          Sorry @krlohnes - if you're not following along we had a snag in 3.2.5 release so the release branches still aren't open. Probably won't see this PR handled until next week some time at this rate.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 Sorry @krlohnes - if you're not following along we had a snag in 3.2.5 release so the release branches still aren't open. Probably won't see this PR handled until next week some time at this rate.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @spmallette Yeah, I'm on the mailing list, I've been following along. No worries.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 @spmallette Yeah, I'm on the mailing list, I've been following along. No worries.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          `GremlinServerWsAndHttpIntegrateTest.java` is a nice validation that all the REST tests work and a reasonable portion websockets tests work, but is there a nicer way to do that? Under this model, every time we add a new test we have to also implement that test method in `GremlinServerWsAndHttpIntegrateTest.java`. We're also not really testing all functionality of websockets as it only does `GremlinServerIntegrateTest`.

          anyway - any thoughts on a better testing model?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 `GremlinServerWsAndHttpIntegrateTest.java` is a nice validation that all the REST tests work and a reasonable portion websockets tests work, but is there a nicer way to do that? Under this model, every time we add a new test we have to also implement that test method in `GremlinServerWsAndHttpIntegrateTest.java`. We're also not really testing all functionality of websockets as it only does `GremlinServerIntegrateTest`. anyway - any thoughts on a better testing model?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          I can probably figure something out with JUnit `Parameters` for the `GremlinServerIntegrateTest` and `GremlinServerHttpIntegrateTest`. I'll have a go at it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 I can probably figure something out with JUnit `Parameters` for the `GremlinServerIntegrateTest` and `GremlinServerHttpIntegrateTest`. I'll have a go at it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          Not sure how I feel about it but just throwing stuff out there to think about....Another option might be to just have a smoke test for the combined channelizer? or maybe all tests should run over your combined channelizer and we just need smoke tests for the specific ones? do you feel like smoke tests are too risky given your changes? Do we need to re-test all functionality over all the channelizers?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 Not sure how I feel about it but just throwing stuff out there to think about....Another option might be to just have a smoke test for the combined channelizer? or maybe all tests should run over your combined channelizer and we just need smoke tests for the specific ones? do you feel like smoke tests are too risky given your changes? Do we need to re-test all functionality over all the channelizers?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          My change can be summed up by

          "Setup the WebSocket stuff and insert the `GremlinEndpointHandler` into the `ChannelPipeline` to serve Http requests when necessary"

          Changes to the `WebSocketChannelizer` and `SaslAuthenticationHandler` would likely be picked up easily enough by simply using the combined channelizer. If there was an issue with the `HttpChannelizer`, it may not be picked up by those tests.

          Running all of the tests from both was mostly for my own sanity. I wanted to make sure I wasn't missing something.

          If you're thinking about minimizing impact from extra tests running in the TP suite, I'd probably suggest running the combined channelizer on WS tests and both on `GremlinServerHttpIntegrateTest`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 My change can be summed up by "Setup the WebSocket stuff and insert the `GremlinEndpointHandler` into the `ChannelPipeline` to serve Http requests when necessary" Changes to the `WebSocketChannelizer` and `SaslAuthenticationHandler` would likely be picked up easily enough by simply using the combined channelizer. If there was an issue with the `HttpChannelizer`, it may not be picked up by those tests. Running all of the tests from both was mostly for my own sanity. I wanted to make sure I wasn't missing something. If you're thinking about minimizing impact from extra tests running in the TP suite, I'd probably suggest running the combined channelizer on WS tests and both on `GremlinServerHttpIntegrateTest`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          ok - let me think about it a bit before you write too much code in any particular direction. we'll also give others some time to chime in in case they have an opinion.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 ok - let me think about it a bit before you write too much code in any particular direction. we'll also give others some time to chime in in case they have an opinion.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          The more i think about this the more I wonder if testing lots of server configurations with the same scenarios is useful. Generally speaking the `Channelizer` isn't responsible for many of the features under test in `GremlinServerIntegrateTest` and there are other tests that are probably testing `Channelizer` capabilities that aren't represented there (maybe more true on master than tp32 - you might need a second PR when we sort this one out).

          I think it would be worth identifying the tests that validate that a `Channelizer` is doing its job right and make a common set of tests that are executed over different server configurations. This would probably mean cherry picking tests from existing ones and moving them to this new test class.

          For example, `shouldBatchResultsByTwos` has nothing to do with the `Channelizer` selection really. That's an integration test for the `OpProcessor` really. On a similar note, `shouldReceiveFailureTimeOutOnScriptEval` is a integration test of the `GremlinExecutor` - such a test would not need to be executed across multiple configurations.

          On the other hand, `shouldStartWithDefaultSettings` is quite generic and would be a good general smoke test. That one could be cherry picked to the new common test class and renamed to "shouldReturnListOfNumbers". We'd probably also want to know if SSL/authentication can be enabled across all `Channelizer` implementations - that's a common feature and one controlled by the `Channelizer`.

          Does that make sense to you at all?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 The more i think about this the more I wonder if testing lots of server configurations with the same scenarios is useful. Generally speaking the `Channelizer` isn't responsible for many of the features under test in `GremlinServerIntegrateTest` and there are other tests that are probably testing `Channelizer` capabilities that aren't represented there (maybe more true on master than tp32 - you might need a second PR when we sort this one out). I think it would be worth identifying the tests that validate that a `Channelizer` is doing its job right and make a common set of tests that are executed over different server configurations. This would probably mean cherry picking tests from existing ones and moving them to this new test class. For example, `shouldBatchResultsByTwos` has nothing to do with the `Channelizer` selection really. That's an integration test for the `OpProcessor` really. On a similar note, `shouldReceiveFailureTimeOutOnScriptEval` is a integration test of the `GremlinExecutor` - such a test would not need to be executed across multiple configurations. On the other hand, `shouldStartWithDefaultSettings` is quite generic and would be a good general smoke test. That one could be cherry picked to the new common test class and renamed to "shouldReturnListOfNumbers". We'd probably also want to know if SSL/authentication can be enabled across all `Channelizer` implementations - that's a common feature and one controlled by the `Channelizer`. Does that make sense to you at all?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          Yeah, I definitely think that makes sense, and it's likely the simplest solution to the tests here.

          I'll make sure the new test hits `shouldStartWithDefaultSettings`, ssl and auth, and keep the test that toggles between the two protocols to make sure they can both be connected to. I think those would be a reasonable check on whether or not this functionality works. I'll ping you once I pick out those tests. If anything else comes to mind, let me know, and I'm happy to add it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 Yeah, I definitely think that makes sense, and it's likely the simplest solution to the tests here. I'll make sure the new test hits `shouldStartWithDefaultSettings`, ssl and auth, and keep the test that toggles between the two protocols to make sure they can both be connected to. I think those would be a reasonable check on whether or not this functionality works. I'll ping you once I pick out those tests. If anything else comes to mind, let me know, and I'm happy to add it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @krlohnes have you done any performance testing to determine if the combined channelizer shows any difference to the independent channelizers? i think that would be worthwhile to know in evaluating this PR. As an added note which I didn't mention before I don't think - this PR will need reference docs, upgrade docs and changelog entries.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 @krlohnes have you done any performance testing to determine if the combined channelizer shows any difference to the independent channelizers? i think that would be worthwhile to know in evaluating this PR. As an added note which I didn't mention before I don't think - this PR will need reference docs, upgrade docs and changelog entries.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @spmallette I did some testing between the http channelizer and the combined one.

          With Basic Auth, the mean latency with 10000 requests for the combined channelizer was 148.3 ms and the standalone channelizer was 148.9 ms.

          Without Basic Auth the mean latency with 10000 request for the combined channelizer was 4.8ms and the standalone channelizer was 4.9 ms

          Given the results for http and that the ChannelPipeline is only manipulated for http, I didn't run any performance tests for WebSockets.

          I'll write up the docs once I'm done refactoring the Channelizer tests.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 @spmallette I did some testing between the http channelizer and the combined one. With Basic Auth, the mean latency with 10000 requests for the combined channelizer was 148.3 ms and the standalone channelizer was 148.9 ms. Without Basic Auth the mean latency with 10000 request for the combined channelizer was 4.8ms and the standalone channelizer was 4.9 ms Given the results for http and that the ChannelPipeline is only manipulated for http, I didn't run any performance tests for WebSockets. I'll write up the docs once I'm done refactoring the Channelizer tests.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          interesting - thanks for that information.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 interesting - thanks for that information.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @spmallete I refactored the channelizer tests and removed some elsewhere when I thought they were appropriately replaced. I'm ultimately not all too familiar with Java NIO. I added it as another channelizer test, but I can delete that test class and the associated code in the abstract test class as it really only runs a single test. I wasn't quite sure what the best way to go with that was. I also added in the doc changes.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 @spmallete I refactored the channelizer tests and removed some elsewhere when I thought they were appropriately replaced. I'm ultimately not all too familiar with Java NIO. I added it as another channelizer test, but I can delete that test class and the associated code in the abstract test class as it really only runs a single test. I wasn't quite sure what the best way to go with that was. I also added in the doc changes.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/618#discussion_r125008894

          — Diff: docs/src/upgrade/release-3.2.x-incubating.asciidoc —
          @@ -39,6 +39,13 @@ it has not been promoted as the primary way to add `IoRegistry` instances to ser

          See: link:https://issues.apache.org/jira/browse/TINKERPOP-1694[TINKERPOP-1694]

          +WsAndHttpChannelizer
          +^^^^^^^^^^^^^^^^^^^^
          +
          +The `WsAndHttpChannelizer` has been added to allow for processing both WebSocket and Http requests on the same
          +port and gremlin server. The `SaslAndHttpBasicAuthenticationHandler` has also been added to service
          +authentication for both protocols in conjunction with the `SimpleAuthenticator`.
          +
          — End diff –

          please include a link to the JIRA here. see other entries for the example.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/618#discussion_r125008894 — Diff: docs/src/upgrade/release-3.2.x-incubating.asciidoc — @@ -39,6 +39,13 @@ it has not been promoted as the primary way to add `IoRegistry` instances to ser See: link: https://issues.apache.org/jira/browse/TINKERPOP-1694[TINKERPOP-1694 ] +WsAndHttpChannelizer +^^^^^^^^^^^^^^^^^^^^ + +The `WsAndHttpChannelizer` has been added to allow for processing both WebSocket and Http requests on the same +port and gremlin server. The `SaslAndHttpBasicAuthenticationHandler` has also been added to service +authentication for both protocols in conjunction with the `SimpleAuthenticator`. + — End diff – please include a link to the JIRA here. see other entries for the example.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/618#discussion_r125010442

          — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java —
          @@ -212,11 +208,6 @@ public Settings overrideSettings(final Settings settings) {
          // Trust ONLY the server cert
          settings.ssl.trustCertChainFile = SERVER_CRT;
          break;

          • case "shouldStartWithDefaultSettings":
              • End diff –

          you removed this test configuration option, but left the test itself? did you mean to do that?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/618#discussion_r125010442 — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java — @@ -212,11 +208,6 @@ public Settings overrideSettings(final Settings settings) { // Trust ONLY the server cert settings.ssl.trustCertChainFile = SERVER_CRT; break; case "shouldStartWithDefaultSettings": End diff – you removed this test configuration option, but left the test itself? did you mean to do that?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/618#discussion_r125021565

          — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/channel/AbstractGremlinChannelizerIntegrateTest.java —
          @@ -0,0 +1,326 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing,
          + * software distributed under the License is distributed on an
          + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + * KIND, either express or implied. See the License for the
          + * specific language governing permissions and limitations
          + * under the License.
          + */
          +package org.apache.tinkerpop.gremlin.server.channel;
          +
          +import org.apache.tinkerpop.gremlin.driver.AuthProperties;
          +import org.apache.tinkerpop.gremlin.driver.Client;
          +import org.apache.tinkerpop.gremlin.driver.Cluster;
          +import org.apache.tinkerpop.gremlin.driver.simple.SimpleClient;
          +import org.apache.tinkerpop.gremlin.server.AbstractGremlinServerIntegrationTest;
          +import org.apache.tinkerpop.gremlin.server.channel.WsAndHttpChannelizer;
          +import org.apache.tinkerpop.gremlin.server.Settings;
          +import org.apache.tinkerpop.gremlin.server.TestClientFactory;
          +
          +import org.apache.http.Consts;
          +import org.apache.http.client.methods.CloseableHttpResponse;
          +import org.apache.http.client.methods.HttpGet;
          +import org.apache.http.client.methods.HttpPost;
          +import org.apache.http.config.Registry;
          +import org.apache.http.config.RegistryBuilder;
          +import org.apache.http.conn.socket.ConnectionSocketFactory;
          +import org.apache.http.conn.ssl.AllowAllHostnameVerifier;
          +import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
          +import org.apache.http.conn.ssl.SSLContextBuilder;
          +import org.apache.http.conn.ssl.TrustSelfSignedStrategy;
          +import org.apache.http.conn.ssl.TrustStrategy;
          +import org.apache.http.entity.StringEntity;
          +import org.apache.http.impl.client.HttpClients;
          +import org.apache.http.impl.client.CloseableHttpClient;
          +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
          +import org.apache.http.util.EntityUtils;
          +import org.apache.tinkerpop.shaded.jackson.databind.JsonNode;
          +import org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper;
          +import org.junit.After;
          +import org.junit.Before;
          +import org.junit.Test;
          +import org.junit.runner.RunWith;
          +import org.junit.runners.Parameterized;
          +import org.junit.rules.ExternalResource;
          +
          +import java.io.File;
          +import java.io.InputStream;
          +import java.security.cert.CertificateException;
          +import java.security.cert.X509Certificate;
          +import java.util.Base64;
          +import java.util.List;
          +
          +import static org.junit.Assert.assertEquals;
          +import static org.junit.Assert.fail;
          +import static org.apache.tinkerpop.gremlin.driver.AuthProperties.Property;
          +
          +abstract class AbstractGremlinServerChannelizerIntegrateTest extends AbstractGremlinServerIntegrationTest {
          — End diff –

          The file name and the class name are different

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/618#discussion_r125021565 — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/channel/AbstractGremlinChannelizerIntegrateTest.java — @@ -0,0 +1,326 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.server.channel; + +import org.apache.tinkerpop.gremlin.driver.AuthProperties; +import org.apache.tinkerpop.gremlin.driver.Client; +import org.apache.tinkerpop.gremlin.driver.Cluster; +import org.apache.tinkerpop.gremlin.driver.simple.SimpleClient; +import org.apache.tinkerpop.gremlin.server.AbstractGremlinServerIntegrationTest; +import org.apache.tinkerpop.gremlin.server.channel.WsAndHttpChannelizer; +import org.apache.tinkerpop.gremlin.server.Settings; +import org.apache.tinkerpop.gremlin.server.TestClientFactory; + +import org.apache.http.Consts; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.config.Registry; +import org.apache.http.config.RegistryBuilder; +import org.apache.http.conn.socket.ConnectionSocketFactory; +import org.apache.http.conn.ssl.AllowAllHostnameVerifier; +import org.apache.http.conn.ssl.SSLConnectionSocketFactory; +import org.apache.http.conn.ssl.SSLContextBuilder; +import org.apache.http.conn.ssl.TrustSelfSignedStrategy; +import org.apache.http.conn.ssl.TrustStrategy; +import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.HttpClients; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; +import org.apache.http.util.EntityUtils; +import org.apache.tinkerpop.shaded.jackson.databind.JsonNode; +import org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.rules.ExternalResource; + +import java.io.File; +import java.io.InputStream; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; +import java.util.Base64; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; +import static org.apache.tinkerpop.gremlin.driver.AuthProperties.Property; + +abstract class AbstractGremlinServerChannelizerIntegrateTest extends AbstractGremlinServerIntegrationTest { — End diff – The file name and the class name are different
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/618#discussion_r125022658

          — Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WebSocketHandlerUtil.java —
          @@ -0,0 +1,38 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing,
          + * software distributed under the License is distributed on an
          + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + * KIND, either express or implied. See the License for the
          + * specific language governing permissions and limitations
          + * under the License.
          + */
          +package org.apache.tinkerpop.gremlin.server.handler;
          +
          +import io.netty.handler.codec.http.HttpMessage;
          +
          +import static io.netty.handler.codec.http.HttpHeaders.Names.UPGRADE;
          +import static io.netty.handler.codec.http.HttpHeaders.Names.CONNECTION;
          +
          +/**
          + * A class to handle common WebSocket operations
          + * @author Keith Lohnes lohnesk@gmail.com
          + */
          +public class WebSocketHandlerUtil {
          — End diff –

          As this is a utility class, please make it final and give it a private constructor. Maybe it doesn't even need to be public?

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/618#discussion_r125022658 — Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WebSocketHandlerUtil.java — @@ -0,0 +1,38 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.server.handler; + +import io.netty.handler.codec.http.HttpMessage; + +import static io.netty.handler.codec.http.HttpHeaders.Names.UPGRADE; +import static io.netty.handler.codec.http.HttpHeaders.Names.CONNECTION; + +/** + * A class to handle common WebSocket operations + * @author Keith Lohnes lohnesk@gmail.com + */ +public class WebSocketHandlerUtil { — End diff – As this is a utility class, please make it final and give it a private constructor. Maybe it doesn't even need to be public?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          I think NIO should have the same capabilities as the WS channelizer. I think you could setup NIO to test the same things WS does.

          Hate to nitpick but could you do a quick pass through your changes and `final` your variables as it's our style.

          I like the "channel" tests. We really needed something unified like that.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 I think NIO should have the same capabilities as the WS channelizer. I think you could setup NIO to test the same things WS does. Hate to nitpick but could you do a quick pass through your changes and `final` your variables as it's our style. I like the "channel" tests. We really needed something unified like that.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/618#discussion_r125040201

          — Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WebSocketHandlerUtil.java —
          @@ -0,0 +1,38 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing,
          + * software distributed under the License is distributed on an
          + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + * KIND, either express or implied. See the License for the
          + * specific language governing permissions and limitations
          + * under the License.
          + */
          +package org.apache.tinkerpop.gremlin.server.handler;
          +
          +import io.netty.handler.codec.http.HttpMessage;
          +
          +import static io.netty.handler.codec.http.HttpHeaders.Names.UPGRADE;
          +import static io.netty.handler.codec.http.HttpHeaders.Names.CONNECTION;
          +
          +/**
          + * A class to handle common WebSocket operations
          + * @author Keith Lohnes lohnesk@gmail.com
          + */
          +public class WebSocketHandlerUtil {
          — End diff –

          fixed

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/618#discussion_r125040201 — Diff: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/WebSocketHandlerUtil.java — @@ -0,0 +1,38 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.server.handler; + +import io.netty.handler.codec.http.HttpMessage; + +import static io.netty.handler.codec.http.HttpHeaders.Names.UPGRADE; +import static io.netty.handler.codec.http.HttpHeaders.Names.CONNECTION; + +/** + * A class to handle common WebSocket operations + * @author Keith Lohnes lohnesk@gmail.com + */ +public class WebSocketHandlerUtil { — End diff – fixed
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/618#discussion_r125039061

          — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/channel/AbstractGremlinChannelizerIntegrateTest.java —
          @@ -0,0 +1,326 @@
          +/*
          + * Licensed to the Apache Software Foundation (ASF) under one
          + * or more contributor license agreements. See the NOTICE file
          + * distributed with this work for additional information
          + * regarding copyright ownership. The ASF licenses this file
          + * to you under the Apache License, Version 2.0 (the
          + * "License"); you may not use this file except in compliance
          + * with the License. You may obtain a copy of the License at
          + *
          + * http://www.apache.org/licenses/LICENSE-2.0
          + *
          + * Unless required by applicable law or agreed to in writing,
          + * software distributed under the License is distributed on an
          + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
          + * KIND, either express or implied. See the License for the
          + * specific language governing permissions and limitations
          + * under the License.
          + */
          +package org.apache.tinkerpop.gremlin.server.channel;
          +
          +import org.apache.tinkerpop.gremlin.driver.AuthProperties;
          +import org.apache.tinkerpop.gremlin.driver.Client;
          +import org.apache.tinkerpop.gremlin.driver.Cluster;
          +import org.apache.tinkerpop.gremlin.driver.simple.SimpleClient;
          +import org.apache.tinkerpop.gremlin.server.AbstractGremlinServerIntegrationTest;
          +import org.apache.tinkerpop.gremlin.server.channel.WsAndHttpChannelizer;
          +import org.apache.tinkerpop.gremlin.server.Settings;
          +import org.apache.tinkerpop.gremlin.server.TestClientFactory;
          +
          +import org.apache.http.Consts;
          +import org.apache.http.client.methods.CloseableHttpResponse;
          +import org.apache.http.client.methods.HttpGet;
          +import org.apache.http.client.methods.HttpPost;
          +import org.apache.http.config.Registry;
          +import org.apache.http.config.RegistryBuilder;
          +import org.apache.http.conn.socket.ConnectionSocketFactory;
          +import org.apache.http.conn.ssl.AllowAllHostnameVerifier;
          +import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
          +import org.apache.http.conn.ssl.SSLContextBuilder;
          +import org.apache.http.conn.ssl.TrustSelfSignedStrategy;
          +import org.apache.http.conn.ssl.TrustStrategy;
          +import org.apache.http.entity.StringEntity;
          +import org.apache.http.impl.client.HttpClients;
          +import org.apache.http.impl.client.CloseableHttpClient;
          +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
          +import org.apache.http.util.EntityUtils;
          +import org.apache.tinkerpop.shaded.jackson.databind.JsonNode;
          +import org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper;
          +import org.junit.After;
          +import org.junit.Before;
          +import org.junit.Test;
          +import org.junit.runner.RunWith;
          +import org.junit.runners.Parameterized;
          +import org.junit.rules.ExternalResource;
          +
          +import java.io.File;
          +import java.io.InputStream;
          +import java.security.cert.CertificateException;
          +import java.security.cert.X509Certificate;
          +import java.util.Base64;
          +import java.util.List;
          +
          +import static org.junit.Assert.assertEquals;
          +import static org.junit.Assert.fail;
          +import static org.apache.tinkerpop.gremlin.driver.AuthProperties.Property;
          +
          +abstract class AbstractGremlinServerChannelizerIntegrateTest extends AbstractGremlinServerIntegrationTest {
          — End diff –

          fixed

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/618#discussion_r125039061 — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/channel/AbstractGremlinChannelizerIntegrateTest.java — @@ -0,0 +1,326 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tinkerpop.gremlin.server.channel; + +import org.apache.tinkerpop.gremlin.driver.AuthProperties; +import org.apache.tinkerpop.gremlin.driver.Client; +import org.apache.tinkerpop.gremlin.driver.Cluster; +import org.apache.tinkerpop.gremlin.driver.simple.SimpleClient; +import org.apache.tinkerpop.gremlin.server.AbstractGremlinServerIntegrationTest; +import org.apache.tinkerpop.gremlin.server.channel.WsAndHttpChannelizer; +import org.apache.tinkerpop.gremlin.server.Settings; +import org.apache.tinkerpop.gremlin.server.TestClientFactory; + +import org.apache.http.Consts; +import org.apache.http.client.methods.CloseableHttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.client.methods.HttpPost; +import org.apache.http.config.Registry; +import org.apache.http.config.RegistryBuilder; +import org.apache.http.conn.socket.ConnectionSocketFactory; +import org.apache.http.conn.ssl.AllowAllHostnameVerifier; +import org.apache.http.conn.ssl.SSLConnectionSocketFactory; +import org.apache.http.conn.ssl.SSLContextBuilder; +import org.apache.http.conn.ssl.TrustSelfSignedStrategy; +import org.apache.http.conn.ssl.TrustStrategy; +import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.HttpClients; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; +import org.apache.http.util.EntityUtils; +import org.apache.tinkerpop.shaded.jackson.databind.JsonNode; +import org.apache.tinkerpop.shaded.jackson.databind.ObjectMapper; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.rules.ExternalResource; + +import java.io.File; +import java.io.InputStream; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; +import java.util.Base64; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; +import static org.apache.tinkerpop.gremlin.driver.AuthProperties.Property; + +abstract class AbstractGremlinServerChannelizerIntegrateTest extends AbstractGremlinServerIntegrationTest { — End diff – fixed
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/618#discussion_r125040344

          — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java —
          @@ -212,11 +208,6 @@ public Settings overrideSettings(final Settings settings) {
          // Trust ONLY the server cert
          settings.ssl.trustCertChainFile = SERVER_CRT;
          break;

          • case "shouldStartWithDefaultSettings":
              • End diff –

          Yeah, I mean to delete that too, thanks. Fixed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/618#discussion_r125040344 — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinServerIntegrateTest.java — @@ -212,11 +208,6 @@ public Settings overrideSettings(final Settings settings) { // Trust ONLY the server cert settings.ssl.trustCertChainFile = SERVER_CRT; break; case "shouldStartWithDefaultSettings": End diff – Yeah, I mean to delete that too, thanks. Fixed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/618#discussion_r125045753

          — Diff: docs/src/upgrade/release-3.2.x-incubating.asciidoc —
          @@ -39,6 +39,13 @@ it has not been promoted as the primary way to add `IoRegistry` instances to ser

          See: link:https://issues.apache.org/jira/browse/TINKERPOP-1694[TINKERPOP-1694]

          +WsAndHttpChannelizer
          +^^^^^^^^^^^^^^^^^^^^
          +
          +The `WsAndHttpChannelizer` has been added to allow for processing both WebSocket and Http requests on the same
          +port and gremlin server. The `SaslAndHttpBasicAuthenticationHandler` has also been added to service
          +authentication for both protocols in conjunction with the `SimpleAuthenticator`.
          +
          — End diff –

          Fixed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/618#discussion_r125045753 — Diff: docs/src/upgrade/release-3.2.x-incubating.asciidoc — @@ -39,6 +39,13 @@ it has not been promoted as the primary way to add `IoRegistry` instances to ser See: link: https://issues.apache.org/jira/browse/TINKERPOP-1694[TINKERPOP-1694 ] +WsAndHttpChannelizer +^^^^^^^^^^^^^^^^^^^^ + +The `WsAndHttpChannelizer` has been added to allow for processing both WebSocket and Http requests on the same +port and gremlin server. The `SaslAndHttpBasicAuthenticationHandler` has also been added to service +authentication for both protocols in conjunction with the `SimpleAuthenticator`. + — End diff – Fixed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @spmallette I fixed the review comments and I think I hit all the places I missed the `final` variables.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 @spmallette I fixed the review comments and I think I hit all the places I missed the `final` variables.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          Okay. I got the NIOChannelizer tests passing now as well, things are `final`ed where appropriate, and I've addressed your PR comments.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 Okay. I got the NIOChannelizer tests passing now as well, things are `final`ed where appropriate, and I've addressed your PR comments.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          All tests pass with `docker/build.sh -t -n -i`

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 All tests pass with `docker/build.sh -t -n -i` VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          I fixed the broken test on tp32. I guess this needs to be rebased again. Not sure how that failing test snuck through review....kinda odd

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 I fixed the broken test on tp32. I guess this needs to be rebased again. Not sure how that failing test snuck through review....kinda odd
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @spmallette I hadn't seen that failure before, but that was from a pretty recent change, ya? I don't think I was up to date with `upstream/tp32`. At any rate, I've rebased the branch on my fork.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 @spmallette I hadn't seen that failure before, but that was from a pretty recent change, ya? I don't think I was up to date with `upstream/tp32`. At any rate, I've rebased the branch on my fork.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          All tests pass with `docker/build.sh -t -n -i`

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 All tests pass with `docker/build.sh -t -n -i` VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user robertdale commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user robertdale commented on the issue: https://github.com/apache/tinkerpop/pull/618 VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          VOTE +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/618 VOTE +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @krlohnes looks like you have all the votes together for this now. it went into tp32 quite well of course, but when i tried to go to master, i hit some problems (tests failing).

          do you mind submitting a second separate pull request to master with these changes? if you do that i can get them both merged at the same time (no need more voting imo).

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 @krlohnes looks like you have all the votes together for this now. it went into tp32 quite well of course, but when i tried to go to master, i hit some problems (tests failing). do you mind submitting a second separate pull request to master with these changes? if you do that i can get them both merged at the same time (no need more voting imo).
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user krlohnes opened a pull request:

          https://github.com/apache/tinkerpop/pull/663

          TINKERPOP-915Add combined handler for Http and Websockets

          TINKERPOP-915(https://issues.apache.org/jira/browse/TINKERPOP-915)

          Most of this is tests. I added an integration test that goes over the
          functionality of both the http and websocket channelizers using the new
          `WsAndHttpChannelizer`. I added an additional test on top of that to
          switch between using WebSockets and http.

          The change itself leverages the existing `WebSocketChannelizer` to
          provide the base pipeline setup. It has everything needed handler-wise
          to service both http and ws connections. The
          `WsAndHttpChannelizerHandler` then detects whether the incoming request
          is a plain http connection or a WebSockets connection. If it's an http
          connection, the channelizer handler swaps out the request handler
          appropriately for whether or not authentication has been enabled.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/krlohnes/tinkerpop add_combined_handler_master

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/663.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #663


          commit efbe426d2e869c80c28c4fee1f8e668e280b33e5
          Author: Keith Lohnes <krlohnes@us.ibm.com>
          Date: 2017-05-30T14:02:54Z

          TINKERPOP-915Add combined handler for Http and Websockets

          TINKERPOP-915(https://issues.apache.org/jira/browse/TINKERPOP-915)

          Most of this is tests. I added an integration test that goes over the
          functionality of both the http and websocket channelizers using the new
          `WsAndHttpChannelizer`. I added an additional test on top of that to
          switch between using WebSockets and http.

          The change itself leverages the existing `WebSocketChannelizer` to
          provide the base pipeline setup. It has everything needed handler-wise
          to service both http and ws connections. The
          `WsAndHttpChannelizerHandler` then detects whether the incoming request
          is a plain http connection or a WebSockets connection. If it's an http
          connection, the channelizer handler swaps out the request handler
          appropriately for whether or not authentication has been enabled.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user krlohnes opened a pull request: https://github.com/apache/tinkerpop/pull/663 TINKERPOP-915 Add combined handler for Http and Websockets TINKERPOP-915 ( https://issues.apache.org/jira/browse/TINKERPOP-915 ) Most of this is tests. I added an integration test that goes over the functionality of both the http and websocket channelizers using the new `WsAndHttpChannelizer`. I added an additional test on top of that to switch between using WebSockets and http. The change itself leverages the existing `WebSocketChannelizer` to provide the base pipeline setup. It has everything needed handler-wise to service both http and ws connections. The `WsAndHttpChannelizerHandler` then detects whether the incoming request is a plain http connection or a WebSockets connection. If it's an http connection, the channelizer handler swaps out the request handler appropriately for whether or not authentication has been enabled. You can merge this pull request into a Git repository by running: $ git pull https://github.com/krlohnes/tinkerpop add_combined_handler_master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/663.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #663 commit efbe426d2e869c80c28c4fee1f8e668e280b33e5 Author: Keith Lohnes <krlohnes@us.ibm.com> Date: 2017-05-30T14:02:54Z TINKERPOP-915 Add combined handler for Http and Websockets TINKERPOP-915 ( https://issues.apache.org/jira/browse/TINKERPOP-915 ) Most of this is tests. I added an integration test that goes over the functionality of both the http and websocket channelizers using the new `WsAndHttpChannelizer`. I added an additional test on top of that to switch between using WebSockets and http. The change itself leverages the existing `WebSocketChannelizer` to provide the base pipeline setup. It has everything needed handler-wise to service both http and ws connections. The `WsAndHttpChannelizerHandler` then detects whether the incoming request is a plain http connection or a WebSockets connection. If it's an http connection, the channelizer handler swaps out the request handler appropriately for whether or not authentication has been enabled.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @spmallette done.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 @spmallette done.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/663

          I'm not sure if you're done with this, but as it sits right now you didn't resolve the compilation problems (and even when you resolve those there are test failures - or maybe i didn't resolve the merges/compilation problems correctly). please run the build with integration tests to see the problems.

          Also, your PR looks a little odd - the commit id is not the same as the one on your other PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/663 I'm not sure if you're done with this, but as it sits right now you didn't resolve the compilation problems (and even when you resolve those there are test failures - or maybe i didn't resolve the merges/compilation problems correctly). please run the build with integration tests to see the problems. Also, your PR looks a little odd - the commit id is not the same as the one on your other PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes closed the pull request at:

          https://github.com/apache/tinkerpop/pull/663

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes closed the pull request at: https://github.com/apache/tinkerpop/pull/663
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/663

          Closed this, misunderstood what was wanted.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/663 Closed this, misunderstood what was wanted.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user krlohnes commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          @spmallette From the other PR on master I'd had open

          >Also, your PR looks a little odd - the commit id is not the same as the one on your other PR.

          I'm not quite sure how to go about not mucking up the history here. I'd cherry-picked and fixed the merge conflicts/test failures but that changes the commit sha.

          I tried merging this branch with a new branch. That fixes the the commit id issue, but adds a merge commit.

          I'm a little stumped on how to approach it from here.

          Show
          githubbot ASF GitHub Bot added a comment - Github user krlohnes commented on the issue: https://github.com/apache/tinkerpop/pull/618 @spmallette From the other PR on master I'd had open >Also, your PR looks a little odd - the commit id is not the same as the one on your other PR. I'm not quite sure how to go about not mucking up the history here. I'd cherry-picked and fixed the merge conflicts/test failures but that changes the commit sha. I tried merging this branch with a new branch. That fixes the the commit id issue, but adds a merge commit. I'm a little stumped on how to approach it from here.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/618

          the merge commit is acceptable in this case. i'd even expect a follow on commit to fix test problems and such. `git merge` should give us as clean a history as we can get i this situation. thanks for asking on how to proceed.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/618 the merge commit is acceptable in this case. i'd even expect a follow on commit to fix test problems and such. `git merge` should give us as clean a history as we can get i this situation. thanks for asking on how to proceed.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/618

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/618

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              spmallette stephen mallette
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development