[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...

2017-01-26 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/1081#discussion_r98087206 --- Diff: lib/java/src/org/apache/thrift/transport/TFramedTransport.java --- @@ -123,6 +123,12 @@ public void consumeBuffer(int len) { readBuffer_

[GitHub] thrift pull request #1081: THRIFT-3854 add a way in java to clear TFramedTra...

2017-01-26 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/1081#discussion_r98087986 --- Diff: lib/java/src/org/apache/thrift/transport/TFastFramedTransport.java --- @@ -164,6 +166,10 @@ public void consumeBuffer(int len

[GitHub] thrift pull request: THRIFT-3756 Improve requiredness documentatio...

2016-03-27 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/961#discussion_r57536860 --- Diff: doc/specs/idl.md --- @@ -126,8 +126,39 @@ A service provides the interface for a set of functionality provided by a Thrift ### Field

[GitHub] thrift pull request: THRIFT-3756 Improve requiredness documentatio...

2016-03-27 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/961#issuecomment-202165692 LGTM @Jens-G - just one question/comment above. Thanks for surfacing this very important part of the spec in the docs! --- If your project is set up for it, you can

[GitHub] thrift pull request: THRIFT-3756 Improve requiredness documentatio...

2016-03-27 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/961#discussion_r57536162 --- Diff: doc/specs/idl.md --- @@ -126,8 +126,39 @@ A service provides the interface for a set of functionality provided by a Thrift ### Field

[GitHub] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...

2016-02-18 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/840#discussion_r53405834 --- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java --- @@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key

[GitHub] thrift pull request: THRIFT-3112 [Java] AsyncMethodCallback should...

2016-02-18 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/840#discussion_r53405396 --- Diff: lib/java/src/org/apache/thrift/async/TAsyncMethodCall.java --- @@ -225,8 +231,13 @@ private void cleanUpAndFireCallback(SelectionKey key

[GitHub] thrift pull request: THRIFT-3622 Fix deprecated uses of std::auto_...

2016-02-16 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/860#issuecomment-184741791 Thanks for taking a look @nsuke, I'm going to abandon this change until I understand the c/c++ build landscape better. --- If your project is set up for it, yo

[GitHub] thrift pull request: THRIFT-3622 Fix deprecated uses of std::auto_...

2016-02-16 Thread jsirois
Github user jsirois closed the pull request at: https://github.com/apache/thrift/pull/860 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is

[GitHub] thrift pull request: THRIFT-3609 Remove or replace TestPortFixture...

2016-02-14 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/869 THRIFT-3609 Remove or replace TestPortFixture.h. Removed since it is no longer used. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jsirois

[GitHub] thrift pull request: THRIFT-3622 Fix deprecated uses of std::auto_...

2016-02-13 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/860 THRIFT-3622 Fix deprecated uses of std::auto_ptr You can merge this pull request into a Git repository by running: $ git pull https://github.com/jsirois/thrift THRIFT-3622 Alternatively you

[GitHub] thrift pull request: THRIFT-3628 Fix lib/cpp/test/TServerIntegrati...

2016-02-13 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/859 THRIFT-3628 Fix lib/cpp/test/TServerIntegrationTest.cpp to use epheme… …ral ports. This was already the case for most of the suite, which, besides the one fixed constructor

[GitHub] thrift pull request: THRIFT-3623 Fix cpp/lib/test/TSSLSocketInterr...

2016-02-13 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/854#discussion_r52832288 --- Diff: lib/cpp/test/TSSLSocketInterruptTest.cpp --- @@ -91,8 +90,6 @@ BOOST_GLOBAL_FIXTURE(GlobalFixtureSSL); BOOST_GLOBAL_FIXTURE(GlobalFixtureSSL

[GitHub] thrift pull request: THRIFT-3626 Fix lib/cpp/test/TSocketInterrupt...

2016-02-12 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/857 THRIFT-3626 Fix lib/cpp/test/TSocketInterruptTest.cpp to use ephemera… …l ports. Instead of using a fixed port, use an ephemeral port to improve robustness and make way for

[GitHub] thrift pull request: THRIFT-3625 Kill unused #include "TestPortFix...

2016-02-12 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/856#issuecomment-183559776 A re-build of the 1st failed Jenkins CI is away here: https://builds.apache.org/job/Thrift-precommit/108/ Failed on a flaky D test. --- If your project is set up

[GitHub] thrift pull request: Kill unused #include "TestPortFixture.h" in l...

2016-02-12 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/856 Kill unused #include "TestPortFixture.h" in lib/cpp/test/TServerTrans… …portTest.cpp. You can merge this pull request into a Git repository by running: $ git pull https://

[GitHub] thrift pull request: THRIFT-3624 Fix lib/cpp/test/TServerSocketTes...

2016-02-12 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/855 THRIFT-3624 Fix lib/cpp/test/TServerSocketTest.cpp to use ephemeral p… …orts. Instead of using a fixed port, use an ephemeral port to improve robustness and make way for

[GitHub] thrift pull request: THRIFT-3621 Fix cpp/lib/test/TSSLSocketInterr...

2016-02-12 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/854 THRIFT-3621 Fix cpp/lib/test/TSSLSocketInterruptTest.cpp to use ephem… …eral ports. Instead of using a fixed port, use an ephemeral port to improve robustness and make way for

[GitHub] thrift pull request: THRIFT-3621 Fix lib/cpp/test/SecurityTest.cpp...

2016-02-12 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/853 THRIFT-3621 Fix lib/cpp/test/SecurityTest.cpp to use ephemeral ports Instead of using a fixed port, use an ephemeral port to improve robustness and make way for parallelizability. You can merge

[GitHub] thrift pull request: THRIFT-2877 Generate hashCode using primitive...

2016-02-12 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/448#issuecomment-183458933 My 2 nonbinding cents: No reasonable client should care about hashCode values, only that the values meet the hashCode contract (this is true of any language). I don&#

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/850#discussion_r52769020 --- Diff: lib/py/test/test_sslsocket.py --- @@ -75,185 +104,176 @@ def __exit__(self, exc_type, exc_value, traceback): class TSSLSocketTest

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/850#discussion_r52766891 --- Diff: lib/py/test/test_sslsocket.py --- @@ -75,185 +104,176 @@ def __exit__(self, exc_type, exc_value, traceback): class TSSLSocketTest

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/850#discussion_r52766421 --- Diff: lib/py/test/test_sslsocket.py --- @@ -42,22 +43,50 @@ CLIENT_KEY = os.path.join(ROOT_DIR, 'test', 'keys', 'clien

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/850#discussion_r52765381 --- Diff: lib/py/test/test_sslsocket.py --- @@ -75,185 +104,176 @@ def __exit__(self, exc_type, exc_value, traceback): class TSSLSocketTest

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/850#discussion_r52765229 --- Diff: lib/py/test/test_sslsocket.py --- @@ -75,185 +104,176 @@ def __exit__(self, exc_type, exc_value, traceback): class TSSLSocketTest

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/850#discussion_r52765133 --- Diff: lib/py/test/test_sslsocket.py --- @@ -42,22 +43,50 @@ CLIENT_KEY = os.path.join(ROOT_DIR, 'test', 'keys', 'clien

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on a diff in the pull request: https://github.com/apache/thrift/pull/850#discussion_r52765017 --- Diff: lib/py/test/test_sslsocket.py --- @@ -42,22 +43,50 @@ CLIENT_KEY = os.path.join(ROOT_DIR, 'test', 'keys', 'clien

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/850#issuecomment-183379947 Noting that Jenkins CI failed on lib/cpp/test/SecurityTest.cpp before getting to test_sslsocket.py. I have a fix for the SecurityTest in #848 that still needs review

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/850#issuecomment-183369497 @nsuke - hoping you have time to review this one --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your

[GitHub] thrift pull request: lib/py/test/test_sslsocket.py is flaky

2016-02-12 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/850 lib/py/test/test_sslsocket.py is flaky Previously a combination of fixed ports, fixed paths and delays was used in all TSSLSocketTest tests that involved making a client-server connection

[GitHub] thrift pull request: lib/cpp/test/SecurityTest is flaky in jenkins...

2016-02-11 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/848#issuecomment-183037515 Noting the re-build of the jenins job - now using `make` instead of `make -j16` is in-flight here: https://builds.apache.org/job/Thrift-precommit/86 --- If your

[GitHub] thrift pull request: lib/cpp/test/SecurityTest is flaky in jenkins...

2016-02-11 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/848#issuecomment-183020644 The jenkins failure was due to concurrency issues building dart bits. I eliminated use of `make -j$(nproc)` in favor of `make` to avoid this. The failure looked like

[GitHub] thrift pull request: lib/cpp/test/SecurityTest is flaky in jenkins...

2016-02-11 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/848 lib/cpp/test/SecurityTest is flaky in jenkins Thrift-precommit build. Previously, the deprecated `linux` symbol was tested, but under both g++ and clang++ using `-std=c++11` this deprecated

[GitHub] thrift pull request: lib/cpp/test/SecurityTest is flaky in jenkins...

2016-02-11 Thread jsirois
Github user jsirois closed the pull request at: https://github.com/apache/thrift/pull/841 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is

[GitHub] thrift pull request: lib/cpp/test/SecurityTest is flaky in jenkins...

2016-02-10 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/841#issuecomment-182506462 Hrm, although it looks like Travis-CI will go green with this change, Jenkins still flaked out. More investigation needed. This PR will get a bit noisy with debug

[GitHub] thrift pull request: lib/cpp/test/SecurityTest is flaky in jenkins...

2016-02-10 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/841 lib/cpp/test/SecurityTest is flaky in jenkins Thrift-precommit build. Previously, the server socket might not be fully closed from a prior test, leading to flaky port bind failures in CI. Avoid

[GitHub] thrift pull request: [Java] AsyncMethodCallback should be typed in...

2016-02-08 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/840#issuecomment-181546218 NB: This is a straw-man at this point that picks up the work started in #500 by @mreve and whose merits is being discussed here: http://markmail.org/message

[GitHub] thrift pull request: [Java] AsyncMethodCallback should be typed in...

2016-02-08 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/840 [Java] AsyncMethodCallback should be typed in generated AsyncIface The parametrization brings the existing actual parametrization with client call implementation objects to the fore and so this

[GitHub] thrift pull request: THRIFT-3112: Typed AsyncMethodCallback with r...

2016-02-08 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/500#issuecomment-181494987 @mreve I've added some explanation on https://issues.apache.org/jira/browse/THRIFT-3112, but this patch should in-fact be typed differently (today). FWICT the t

[GitHub] thrift pull request: `make check` hangs in go tests

2016-02-03 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/833#issuecomment-179569186 Looks like python tests are broken or flaky: https://travis-ci.org/apache/thrift/jobs/106875786

[GitHub] thrift pull request: THRIFT-3578 Make THeaderTransport detect TCom...

2016-02-03 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/819#issuecomment-179552975 @nsuke - could you kill the 2 hung shards in https://travis-ci.org/apache/thrift/builds/106198835 ? They >24hrs old and are blocking the Apache TravisCI pipel

[GitHub] thrift pull request: Fix hang in go tests.

2016-02-03 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/833#issuecomment-179546686 @dimiro1 - if you have a chance to review, I'd be grateful. This is re #785 and a test hang under `make check` that I bisected to it. --- If your project is s

[GitHub] thrift pull request: Fix hang in go tests.

2016-02-03 Thread jsirois
Github user jsirois commented on the pull request: https://github.com/apache/thrift/pull/833#issuecomment-179546030 Tested with `make check`, which no-longer hangs. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your

[GitHub] thrift pull request: Fix hang in go tests.

2016-02-03 Thread jsirois
GitHub user jsirois opened a pull request: https://github.com/apache/thrift/pull/833 Fix hang in go tests. Previously tests would hang in a forground call to `server.Serve()`. Kill this call since `server.Listen(); go server.AcceptLoop()` is already performed and is the