You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by krlohnes <gi...@git.apache.org> on 2017/12/29 14:35:43 UTC
[GitHub] tinkerpop pull request #767: TINKERPOP-1858: HttpChannelizer Regression: Doe...
GitHub user krlohnes opened a pull request:
https://github.com/apache/tinkerpop/pull/767
TINKERPOP-1858: HttpChannelizer Regression: Does not create specified AuthenticationHandler
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/krlohnes/tinkerpop TP-1858
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tinkerpop/pull/767.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 #767
----
commit 4404a75fb23092cf0d1e5aa96c9afc2ac90cd041
Author: Keith Lohnes <lo...@...>
Date: 2017-12-29T14:31:07Z
TINKERPOP-1858: HttpChannelizer Regression: Does not create specified
AuthenticationHandler
----
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/767
@spmallette Yeah, it looks like it happened with the `Merge branch TP32` commit. If you look [here](https://github.com/apache/tinkerpop/commit/960fdc11399590280522189b08727e90cd9b629a#diff-822877f6fd92f5cd4b57fa2167f92928R74) it has `new HttpBasicAuthenticationHandler()` whereas the [previous commit](https://github.com/apache/tinkerpop/commit/eae4101c5fb7b8b976c5898a12e180ee23e50269#diff-822877f6fd92f5cd4b57fa2167f92928R74) uses `instantiateAuthenticationHandler(settings.authentication)`
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/767
@spmallette I'm not seeing any integration test failures.
```
[INFO]
[INFO] --- maven-failsafe-plugin:2.20:verify (verify-integration-test) @ gremlin-server ---
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 18:16 min
[INFO] Finished at: 2018-01-08T14:50:13-05:00
[INFO] Final Memory: 40M/428M
[INFO] ------------------------------------------------------------------------
```
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/767
Failed docker...
```
docker/build.sh -i -t
[ERROR] Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 13.448 s <<< FAILURE! - in org.apache.tinkerpop.gremlin.server.channel.HttpChannelizerIntegrateTest
[ERROR] shouldBreakOnInvalidAuthenticationHandler(org.apache.tinkerpop.gremlin.server.channel.HttpChannelizerIntegrateTest) Time elapsed: 2.63 s <<< ERROR!
java.lang.Exception: Unexpected exception, expected<org.apache.http.NoHttpResponseException> but was<java.net.SocketException>
at org.apache.tinkerpop.gremlin.server.channel.HttpChannelizerIntegrateTest.shouldBreakOnInvalidAuthenticationHandler(HttpChannelizerIntegrateTest.java:47)
```
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/767
Is it just the wrong expected exception in the test?
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/767
@spmallette As far as criticality, that's fairly dependent on the usage of custom authentication handlers in the wild. It's not impacting my current work or any of the work I did at IBM Compose right now, but it breaks anyone using custom http authentication (e.g. the JanusGraph issue linked to this), so that's pretty bad. That, however, only impacts JanusGraph master right now that I know of though, so it's not impacting an official release there at the moment. So not too bad if there's a patch version planned FSVO soon. I wish I'd caught it before the last one that just went out.
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the issue:
https://github.com/apache/tinkerpop/pull/767
VOTE: +1
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/767
I added in the CHANGELOG entry
---
[GitHub] tinkerpop pull request #767: TINKERPOP-1858: HttpChannelizer Regression: Doe...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/767#discussion_r160843990
--- Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/channel/HttpChannelizerIntegrateTest.java ---
@@ -18,15 +18,40 @@
*/
package org.apache.tinkerpop.gremlin.server.channel;
-
+import org.apache.http.NoHttpResponseException;
import org.apache.tinkerpop.gremlin.server.auth.SimpleAuthenticator;
import org.apache.tinkerpop.gremlin.server.Settings;
+import org.junit.Test;
+
import java.util.Map;
import java.util.HashMap;
+import java.net.SocketException;
public class HttpChannelizerIntegrateTest extends AbstractGremlinServerChannelizerIntegrateTest {
+ @Override
+ public Settings overrideSettings(final Settings settings) {
+ super.overrideSettings(settings);
+ final String nameOfTest = name.getMethodName();
+ if (nameOfTest.equals("shouldBreakOnInvalidAuthenticationHandler") ) {
+ settings.authentication = getAuthSettings();
+ settings.authentication.authenticationHandler = "Foo.class";
+ }
+ return settings;
+ }
+
+ @Test
+ public void shouldBreakOnInvalidAuthenticationHandler() throws Exception {
+ final CombinedTestClient client = new CombinedTestClient(getProtocol());
+ try {
+ client.sendAndAssert("2+2", 4);
--- End diff --
this line should be followed by:
```java
fail("Should throw exception");
```
like we don't expect that `sendAndAssert` to actually work, right?
---
[GitHub] tinkerpop pull request #767: TINKERPOP-1858: HttpChannelizer Regression: Doe...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on a diff in the pull request:
https://github.com/apache/tinkerpop/pull/767#discussion_r160963168
--- Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/channel/HttpChannelizerIntegrateTest.java ---
@@ -18,15 +18,40 @@
*/
package org.apache.tinkerpop.gremlin.server.channel;
-
+import org.apache.http.NoHttpResponseException;
import org.apache.tinkerpop.gremlin.server.auth.SimpleAuthenticator;
import org.apache.tinkerpop.gremlin.server.Settings;
+import org.junit.Test;
+
import java.util.Map;
import java.util.HashMap;
+import java.net.SocketException;
public class HttpChannelizerIntegrateTest extends AbstractGremlinServerChannelizerIntegrateTest {
+ @Override
+ public Settings overrideSettings(final Settings settings) {
+ super.overrideSettings(settings);
+ final String nameOfTest = name.getMethodName();
+ if (nameOfTest.equals("shouldBreakOnInvalidAuthenticationHandler") ) {
+ settings.authentication = getAuthSettings();
+ settings.authentication.authenticationHandler = "Foo.class";
+ }
+ return settings;
+ }
+
+ @Test
+ public void shouldBreakOnInvalidAuthenticationHandler() throws Exception {
+ final CombinedTestClient client = new CombinedTestClient(getProtocol());
+ try {
+ client.sendAndAssert("2+2", 4);
--- End diff --
Fixed
---
[GitHub] tinkerpop pull request #767: TINKERPOP-1858: HttpChannelizer Regression: Doe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tinkerpop/pull/767
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/767
when i do:
```text
$ mvn clean install && mvn verify -pl gremlin-server -DskipIntegrationTests=false
```
i get this failure consistently:
```text
[ERROR] Errors:
[ERROR] HttpChannelizerIntegrateTest.shouldBreakOnInvalidAuthenticationHandler ยป Unex...
[INFO]
[ERROR] Tests run: 204, Failures: 0, Errors: 1, Skipped: 14
[INFO]
[INFO]
[INFO] --- revapi-maven-plugin:0.8.0:check (default) @ gremlin-server ---
[INFO]
[INFO] --- maven-failsafe-plugin:2.20:verify (verify-integration-test) @ gremlin-server ---
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 16:52 min
[INFO] Finished at: 2018-01-09T11:09:03-05:00
[INFO] Final Memory: 49M/725M
[INFO] ------------------------------------------------------------------------
```
from the test output it says:
```text
-------------------------------------------------------------------------------
Test set: org.apache.tinkerpop.gremlin.server.channel.HttpChannelizerIntegrateTest
-------------------------------------------------------------------------------
Tests run: 5, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 12.773 s <<< FAILURE! - in org.apache.tinkerpop.gremlin.server.channel.HttpChannelizerIntegrateTest
shouldBreakOnInvalidAuthenticationHandler(org.apache.tinkerpop.gremlin.server.channel.HttpChannelizerIntegrateTest) Time elapsed: 2.46 s <<< ERROR!
java.lang.Exception: Unexpected exception, expected<org.apache.http.NoHttpResponseException> but was<java.net.SocketException>
at org.apache.tinkerpop.gremlin.server.channel.HttpChannelizerIntegrateTest.shouldBreakOnInvalidAuthenticationHandler(HttpChannelizerIntegrateTest.java:47)
```
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/767
VOTE +1
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/767
a bad merge - thanks. how bad/critical do you rate this problem?
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/767
I'm getting test failures when running Gremlin Server integration tests. @krlohnes is that a problem for you?
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/767
Ok - thanks for your thoughts. I can't say we have a release date for 3.3.2 in mind yet. I'd say it was a couple of months away at least based on how we typically do releases.
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/767
@dkuppitz or @robertdale could you guys give this a try? maybe it's just me for some reason?
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/767
I've seen this kind of thing happen before and we usually handle the dual exception the way that you have it there by catching either exception or catching generically and asserting either one as acceptable.
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/767
@spmallette I rebased on top of upstream/master and ran
`mvn clean install && mvn verify -pl gremlin-server -DskipIntegrationTests=false`
And it was successful
```
[WARNING] Tests run: 204, Failures: 0, Errors: 0, Skipped: 14
[INFO]
[INFO]
[INFO] --- revapi-maven-plugin:0.8.0:check (default) @ gremlin-server ---
[INFO]
[INFO] --- maven-failsafe-plugin:2.20:verify (verify-integration-test) @ gremlin-server ---
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 17:58 min
[INFO] Finished at: 2018-01-09T12:05:54-05:00
[INFO] Final Memory: 56M/681M
[INFO] ------------------------------------------------------------------------
```
Running on OSX and jdk 1.8u152
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by robertdale <gi...@git.apache.org>.
Github user robertdale commented on the issue:
https://github.com/apache/tinkerpop/pull/767
VOTE +1
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by krlohnes <gi...@git.apache.org>.
Github user krlohnes commented on the issue:
https://github.com/apache/tinkerpop/pull/767
@robertdale `NoHttpResponseException` was the exception that I was getting when I wrote the test and ran it locally (OSX). It seems to be a different error in Docker. In Docker I get the `java.net.SocketException`, just like both of you got.
I can change it so it only passes in Docker or I can change the test body to something like.
```java
try {...}
catch(SocketException | NoHttpResponseException e){}
finally {...}
```
I prefer the `try...catch`, but I will defer to you and @spmallette on that.
---
[GitHub] tinkerpop issue #767: TINKERPOP-1858: HttpChannelizer Regression: Does not c...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the issue:
https://github.com/apache/tinkerpop/pull/767
Hi @krlohnes - thanks for this. Did you happen to research the point where this regression occurred?
---