[GitHub] thrift issue #1523: Fix condition where TSimpleServer could exit AcceptLoop(...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1523 THRIFT-4537 ---
[GitHub] thrift issue #1523: Fix condition where TSimpleServer could exit AcceptLoop(...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1523 Thanks for checking, we need to stabilize these CI builds so less effort is spent figuring out if a change is at least somewhat ok. ---
[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1522 That is an intermittent test failure that I thought I resolved. :) it is C++ only ---
[GitHub] thrift pull request #1522: THRIFT-4530: add @Nullable annotations to generat...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1522#discussion_r177720513 --- Diff: lib/java/src/org/apache/thrift/annotations/Nullable.java --- @@ -0,0 +1,33 @@ +/* + * 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.thrift.annotations; --- End diff -- Any reason why java language uses "annotation" but you selected "annotations" for thrift? Seems like we should be using "annotation". ---
[GitHub] thrift issue #1522: THRIFT-4530: add @Nullable annotations to generated Java...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1522 Yes there is an issue that appears in builds where lisp cannot find a file it just supposedly wrote. Lisp is new to the project, this is an ongoing thing. ---
[GitHub] thrift issue #1518: THRIFT-4342: update ruby tests to use rspec 3, updated a...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1518 Closing, will re-open once the dist jobs are working. ---
[GitHub] thrift pull request #1518: THRIFT-4342: update ruby tests to use rspec 3, up...
Github user jeking3 closed the pull request at: https://github.com/apache/thrift/pull/1518 ---
[GitHub] thrift pull request #1518: THRIFT-4342: update ruby tests to use rspec 3, up...
GitHub user jeking3 opened a pull request: https://github.com/apache/thrift/pull/1518 THRIFT-4342: update ruby tests to use rspec 3, updated all dependencies for ruby You can merge this pull request into a Git repository by running: $ git pull https://github.com/jeking3/thrift THRIFT-4342 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1518.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 #1518 commit 0856cfcd00f611522a2c869b754472795490 Author: James E. King III <jking@...> Date: 2018-03-23T00:50:23Z THRIFT-4342: update ruby tests to use rspec 3, updated all dependencies for ruby ---
[GitHub] thrift issue #1515: add connect timeout, support accurate send timeout
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1515 The new style changes and checks are pretty recent, based on work done by @RobberPhex. ---
[GitHub] thrift issue #1515: add connect timeout, support accurate send timeout
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1515 Fortunately these changes are confined to one file so reviewing it isn't too bad - hopefully they pass the new php formatting tests in the sca build. ---
[GitHub] thrift issue #1515: add connect timeout, support accurate send timeout
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1515 Please squash your changes to a single commit if possible. Thanks. ---
[GitHub] thrift issue #1515: add connect timeout, support accurate send timeout
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1515 THRIFT-4171 ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 Looks good, will merge in the morning. ---
[GitHub] thrift pull request #1514: THRIFT-4525: add ruby cross test ssl support
GitHub user jeking3 opened a pull request: https://github.com/apache/thrift/pull/1514 THRIFT-4525: add ruby cross test ssl support You can merge this pull request into a Git repository by running: $ git pull https://github.com/jeking3/thrift THRIFT-4525 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1514.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 #1514 ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1269 Remember to resolve https://issues.apache.org/jira/browse/THRIFT-4187 as fixed in 0.12.0. ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 This squash was not done properly. It includes all of the master commits in it. You want to start with an updated master: git checkout master git pull Then you want to branch: git checkout -b THRIFT-4489-squashed Then you want to merge with squash, pulling anything in uds-nodejs over to THRIFT-4489-squashed git merge --squash uds-nodejs Then commit and add your description for your changes. Then push that branch and let me know when it is in your fork, and I will pull it and merge it. No need to update this PR or open a new one. ---
[GitHub] thrift pull request #1513: THRIFT-4358: add unix domain socket option to rub...
GitHub user jeking3 opened a pull request:
https://github.com/apache/thrift/pull/1513
THRIFT-4358: add unix domain socket option to ruby cross tests
This is the first time I've tried to use ruby with any seriousness. I like
the unit test framework (rspec) -
very descriptive and the mocking is so damn easy compared to C++.
In addition to adding domain socket support to the test harness for client
and server (there was already an implementation in the library), the ruby test
server has a new line of code that's neat:
puts "Starting TestServer #{@server.to_s}"
which turns into, for example:
Starting TestServer
threaded(server(binary(buffered(domain(/tmp/ThriftTest.thrift.57621)
or for a socket test:
Starting TestServer threaded(server(compact(framed(socket(:36653)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jeking3/thrift THRIFT-4358
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1513.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 #1513
commit 971719d13e84aac03037b4afae4c7916661ee371
Author: James E. King III <jking@...>
Date: 2018-03-20T19:06:08Z
THRIFT-4358: add unix domain socket option to ruby cross tests
---
[GitHub] thrift issue #1512: Fix for THRIFT-4524
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1512 Build failures appear unrelated to the change. ---
[GitHub] thrift issue #1511: THRIFT-4476: Typecasting problem on double list items, e...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1511 Thanks for the squash. We'll get this merged. ---
[GitHub] thrift issue #1084: THRIFT-3773 Swift 3 Native Library
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1084 Okay, so I could merge this, but I need to know (since I've never used cocoa, swift, or any of the apple stuff really): * Are there breaking changes? * Are they documented? * Is it already running in the cross test or is that future work? * Can this be exercised in the Travis CI build environment? (make check, make cross?) * Can I add swift support to Ubuntu Xenial and Artful? * What minimum and maximum versions are known supported? * Has the LANGUAGES.md file been updated? * Has the build/docker/README.md file been updated? * Is this supposed to replace the cocoa implementation? Should that be removed? I can make some of these changes as part of finalization. Thanks. ---
[GitHub] thrift issue #972: THRIFT-3770 Implement Python 3.4+ asyncio support
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/972 @jfarrell please close this; the author indicated no interest in finishing the work due to the team's past inaction on pull request management. ---
[GitHub] thrift issue #798: THRIFT-3560: C++: declared TTransport::isOpen() and TTran...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/798 I am okay making this change for 0.12.0; if you can rebase, squash, and fix up the issues and force push, and if there are any other methods that need the same const correctness, let's do those too. ---
[GitHub] thrift issue #782: THRIFT-2790 thrift -gen all => an option to generate all ...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/782 Should this be moved forward or closed? ---
[GitHub] thrift issue #744: update TSocket.php
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/744 @lihanharry please move this forward or it will eventually be closed due to inactivity. @RobberPhex any concerns with this code? ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1269 I'd suggest waiting to see how the CI build fares given a change was made. So today is a distinct possibility. Also, you can commit your own changes. There's no rule against it. :) ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 Okay so, here's the deal. This pull request has 9 commits and one of them is a merge. If you want your name on the commit tag you will need to squash and resubmit with a single commit and then I can cherry-pick it over and push it. Otherwise if you don't care, I can squash and merge it, but my name will end up in the "Author" field of the commit, and I will include your name in the content of the pull request, i.e.: ``` commit 496568e57a17889f484c6275078bef2bcb43382e (HEAD -> master) Author: James E. King III <jk...@apache.org> Date: Mon Mar 19 16:16:37 2018 -0400 THRIFT-4489: Add UDS support for nodejs thrift client Patch: Daniel Shih <hoting...@gmail.com> Client: nodejs This closes #1491 ``` Let me know what you want to do. I'm fine either way. ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 I get an error that may be unrelated when I run cross tests on --server nodejs locally. This one test failes sporadically and I don't know if it could be related to these changes; the client has to be killed in this scenario, I don't know if it is because the server is unresponsive or not. I may just disable this one test for the merge. ``` === *** Following 1 failures were unexpected ***: If it is introduced by you, please fix it before submitting the code. === server-client: protocol: transport: result: nodejs-cpp json buffered-ip-ssl failure(timeout) === Unexpected failures are logged to test/log/unexpected_failures.log You can browse results at: file:///thrift/src/test/index.html # If you use Chrome, run: # cd /thrift/src # python -m http.server 8001 # then browse: # http://localhost:8001/test/ Full log for each test is here: test/log/server_client_protocol_transport_client.log test/log/server_client_protocol_transport_server.log 1 failed of 193 tests in total. Test execution took 30.1 seconds. Mon Mar 19 19:40:15 2018 root@3d39d5d9eb3c:/thrift/src# tail test/log/nodejs-cpp_json_buffered-ip-ssl_client.log testBinary(siz = 4096) testBinary(siz = 8192) testBinary(siz = 16384) testBinary(siz = 32768) testBinary(siz = 65536) testBinary(siz = 131072) === Return code: -15 (negative values indicate kill by signal) Test execution took 8.0 seconds. Mon Mar 19 19:40:11 2018 root@3d39d5d9eb3c:/thrift/src# tail test/log/nodejs-cpp_json_buffered-ip-ssl_server.log Mon Mar 19 19:40:03 2018 Executing: node server.js --type=tcp --protocol=json --transport=buffered --ssl --port=39155 Directory: /thrift/src/lib/nodejs/test config:delay: 5 config:timeout: 8 === === Return code: -1 (negative values indicate kill by signal) Test execution took 8.3 seconds. Mon Mar 19 19:40:11 2018 ``` ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 I am running a cross test on this locally; if it passes I will merge it, so don't take any action. ---
[GitHub] thrift issue #1289: fix TypeError when a py3 str is passed to the function.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1289 @nsuke there must be some common wisdom and/or tooling to facilitate the conversion from py2 to py3 in this regard - what have other projects done when facing this issue? ---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 Yes, however, it needs to be squashed to a single commit. I would prefer if you did that, but I can do it if you really need me to. It's just a lot more work for the committers to do that, and we have a lot of work we're doing already. Looks like a good change though - thanks for that! Clearly you care about your doubles a lot. https://softwareengineering.stackexchange.com/questions/263164/why-squash-git-commits-for-pull-requests ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 Perhaps; my PR for THRIFT-4515 improves the stability and coverage of crosstest by adding more controlled server shutdown, and it passed tests (I've run it many times on my local fork as well). I just merged it into master, so if you want to squash to a single commit and rebase, let's give it a try. ---
[GitHub] thrift issue #1509: THRIFT-4515: cross server test improvement: graceful tes...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1509 The build failure in the artful check job is in lib/d unit tests and is a sporadic issue that needs to be investigated, but will not hold up this work. ---
[GitHub] thrift pull request #1509: THRIFT-4515: cross server test improvement: grace...
GitHub user jeking3 opened a pull request: https://github.com/apache/thrift/pull/1509 THRIFT-4515: cross server test improvement: graceful test server shutdown ### Problem ### Test servers are not shut down gracefully - they are SIGKILLed once the client exits. ### Solution ### Try using SIGINT first, capture the result code and process it properly, SIGKILL if the process seems unresponsive. ### Also ### - Better subprocess management in cross test requires Python 3.3 or later to run the suite. - tests.json can declare "stop_signal": in the server stanza to customize this behavior - Added signal handling to C++, D, and Perl test servers to stop gracefully. - Cleaned up some C++ library compiler warnings. - Fixed C++ TSocket error descriptions for domain sockets to use the path instead of host/port. - Fixed an intermittent issue in concurrency_test. - Perl server had no "stop()" mechanism which was easy to add. - Updated SBCL (lisp) to 1.4.5 to try and eliminate these "file not found" sporadic build issues. You can merge this pull request into a Git repository by running: $ git pull https://github.com/jeking3/thrift THRIFT-4515 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1509.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 #1509 commit f1905707c0f7be34e811d2dc4af50a6e06a01364 Author: James E. King III <jking@...> Date: 2018-03-16T20:07:42Z THRIFT-4515: cross server test improvement: graceful test server shutdown commit 8e116b9af11dc1533725f7806f73ffadff442524 Author: James E. King III <jking@...> Date: 2018-03-19T12:16:51Z THRIFT-82: move to SBCL 1.4.5 (hopefully will address 1.4.4 sporadic build errors) ---
[GitHub] thrift pull request #1496: THRIFT-4476: Typecasting problem on list items (+...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1496#discussion_r175276394 --- Diff: build/appveyor/MSVC-appveyor-build.bat --- @@ -39,7 +39,7 @@ CD "%BUILDDIR%" || EXIT /B -DWITH_PYTHON=%WITH_PYTHON% ^ -DWITH_%THREADMODEL%THREADS=ON ^ -DWITH_SHARED_LIB=OFF ^ --DWITH_STATIC_LIB=ON|| EXIT /B +-DWITH_STATIC_LIB=ON ^|| EXIT /B --- End diff -- The carat addition looks wrong to me; it belongs on the end of a string, so in this case I think it does nothing at all? ---
[GitHub] thrift pull request #1496: THRIFT-4476: Typecasting problem on list items (+...
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1496#discussion_r175276410
--- Diff: compiler/cpp/src/thrift/generate/t_generator.h ---
@@ -268,6 +271,30 @@ class t_generator {
return out.str();
}
+ const std::string emit_double_as_string(const double value) {
+ std::stringstream double_output_stream;
+ // sets the maximum precision:
http://en.cppreference.com/w/cpp/io/manip/setprecision
+ // sets the output format to fixed:
http://en.cppreference.com/w/cpp/io/manip/fixed (not in scientific notation)
+ double_output_stream <<
std::setprecision(std::numeric_limits::digits10 + 1);
+
+ #ifdef _MSC_VER
+ // strtod is broken in MSVC compilers older than 2015, so
std::fixed fails to format a double literal.
+ // more details:
https://blogs.msdn.microsoft.com/vcblog/2014/06/18/
+ //
c-runtime-crt-features-fixes-and-breaking-changes-in-visual-studio-14-ctp1/
+ // and
+ //
http://www.exploringbinary.com/visual-c-plus-plus-strtod-still-broken/
+ #if _MSC_VER >= MSC_2015_VER
+ double_output_stream << std::fixed;
+ #endif
+ #else
+ double_output_stream << std::fixed;
--- End diff --
Did you mean both cases to be the same? Both use std::fixed; should one be
using std::scientific?
---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 If there's a way to fix it in the compiler so that it works properly, that's much better. Having compiler-version-specific branches in the thrift compiler to handle floating point oddities like this is fine. Carrying the C++ compiler identifier down through the build system and pass into the tests as an argument/option/variable should really be avoided. ---
[GitHub] thrift issue #1507: THRIFT-4516: Fix "go vet" warnings for Go 1.10
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1507 Go ahead and merge. After you merge, I update my fork and kick a build on my account to refresh the images. It's an optimization that saves ~10 minutes per build job. ---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 You may be missing my point here. Regardless of the compiler used to build the thrift compiler, whether it is gcc-4.6, clang-6.0, MSVC 2010, MSVC 2013, MSVC 2017, the floating constants emitted from the thrift compiler should be the same. If they are not, I recommend fixing the thrift compiler and NOT disabling the new tests which caught an important issue. ---
[GitHub] thrift issue #1507: THRIFT-4516: Fix "go vet" warnings for Go 1.10
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1507 I'll keep an eye on this. Right now the docker images on docker.io have to be generated manually every time they are updated, otherwise the build jobs take more time than they should. ---
[GitHub] thrift pull request #1507: THRIFT-4516: Fix "go vet" warnings for Go 1.10
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1507#discussion_r174810986
--- Diff: lib/go/test/dontexportrwtest/compile_test.go ---
@@ -29,10 +28,10 @@ import (
func TestReadWriteMethodsArePrivate(t *testing.T) {
// This will only compile if read/write methods exist
s := NewTestStruct()
- fmt.Sprintf("%v", s.read)
- fmt.Sprintf("%v", s.write)
+ _ = s.read
--- End diff --
Nice.. I had done everything else but I had no idea how to fix this in
golang. Thanks!
---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 Need to squash to one commit, ideally, to make a merge easier, and fix the issue causing older MSVC compilers (which people still use...) to generate different floating constants than other compilers. There could be a compiler flag, or a way to fix it in the compiler. It's great that we added tests to verify the output of the compiler; we shouldn't disable the new tests just because the C++ compiler is different. ---
[GitHub] thrift pull request #1496: THRIFT-4476: Typecasting problem on list items (+...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1496#discussion_r174771623 --- Diff: test/DoubleConstantsTest.thrift --- @@ -0,0 +1,17 @@ +namespace java thrift.test +namespace cpp thrift.test + +// more tests on double constants (precision and type checks) --- End diff -- There's probably another way to resolve this, such as fixing a compiler flag being passed to older MSVC2013 compilers. I'll swing back around to this one when I can. I don't like the notion that we say, "because the compiler was built with MSVC2013, let's disable a test". Shouldn't any thrift compiler, regardless of the C++ compiler used to create it, produce working thrift generated code that has the same values as any other thrift compiler? ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 Please rebase on master one more time, most of the build issues are gone now. ---
[GitHub] thrift pull request #1496: THRIFT-4476: Typecasting problem on list items (+...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1496#discussion_r174495089 --- Diff: test/DoubleConstantsTest.thrift --- @@ -0,0 +1,17 @@ +namespace java thrift.test +namespace cpp thrift.test + +// more tests on double constants (precision and type checks) --- End diff -- Further evidence that disabling the test in Java is a bad idea: why don't you need to disable Javascript if the compiler is the cause? ---
[GitHub] thrift pull request #1496: THRIFT-4476: Typecasting problem on list items (+...
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1496#discussion_r174494320
--- Diff: lib/java/gradle/unitTests.gradle ---
@@ -68,6 +68,20 @@ test {
include '**/Test*.class'
exclude '**/Test*\$*.class'
+println 'Check if msvc.profile exists'
--- End diff --
It seems to me this fix is in the wrong place. I don't like having the C++
compiler type passed down into the java build so it can disable a test. The
compiler code be modified somehow.
---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 Thanks for dealing with the relatively unstable CI. Our project has support for 25 languages, so needless to say without a fixed build environment (which is impossible, since packages change over time), the build becomes unstable pretty quickly without constant TLC. Please squash this to a single commit to prepare it for merge. In addition, I will need to make sure this test passes: ``` === *** Following 1 failures were unexpected ***: If it is introduced by you, please fix it before submitting the code. === server-client: protocol: transport: result: hs-csharp json framed-ip failure(1) === Unexpected failures are logged to test/log/unexpected_failures.log You can browse results at: file:///thrift/src/test/index.html # If you use Chrome, run: # cd /thrift/src # python -m http.server 8001 # then browse: # http://localhost:8001/test/ Full log for each test is here: test/log/server_client_protocol_transport_client.log test/log/server_client_protocol_transport_server.log 1 failed of 3948 tests in total. Test execution took 1966.1 seconds. Wed Mar 14 13:26:25 2018 ``` ---
[GitHub] thrift issue #1505: THRIFT-4513: Fixing java thrift compiler to generate con...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1505 Looks like a dlang issue that pops up from time to time. Just to satisfy myself this change is okay I need to look at generated thrift file comparisons from before and after this change. I'll do that locally. ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 @jfarrell can you please give a final say on this? ---
[GitHub] thrift issue #1480: [nodejs] Fix issue with connection failures silently fai...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1480 @bananer apart from missing any testing, is this a reasonable enhancement? ---
[GitHub] thrift issue #1058: Node.js: Set/unset client seqid for json_protocol and co...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1058 In general I want to make sure all clients send a seqid and all servers put that seqid in the result. Without this we won't be able to have clients that can have multiple outstanding requests in the future. ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1269 This needs to be rebased and completed. ---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 I put the MSVC2013 job back in to make sure we had some backwards compatibility... glad I did. I wonder if we have 32-bit issues with double in general in thrift? ---
[GitHub] thrift issue #1380: Support x64 build mode on Windows with DMD v2.076.0.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1380 THRIFT-3458 is also related, and the approach in THRIFT-3458 would allow us to add the apache git repo to dub directly so we should move in that direction, but still try to use dub to build in lib/d, test/d, tutorial/d. I closed THRIFT-4504 as a duplicate of THRIFT-3458. If we can get this PR to the point where there is a dub file at the top level of the repository that builds lib/d, that will allow us to add the apache repo to the dub registry and have it build and publish a dub package. ---
[GitHub] thrift issue #1447: THRIFT-4431 Ruby library: Finish http connection after f...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1447 @lompy will you be able to carry this forward? ---
[GitHub] thrift issue #1479: THRIFT-4474: generate PHP code use PSR-4 style default
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1479 Any other php folks out there want to comment or review on the breaking change here? It looks like the strategy was to change the generator and provide a flag to allow folks to generate older style code for backwards compatibility. ---
[GitHub] thrift issue #1261: THRIFT-4382: Replace the use of Indirect Object Syntax c...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1261 If you are able to finish this up and do the tutorial code, or if you are not able to complete this, please let me know. ---
[GitHub] thrift issue #1141: support for timeout in node http connection
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1141 @bananer this would be a good candidate to pick up and run with. ---
[GitHub] thrift issue #1058: Node.js: Set/unset client seqid for json_protocol and co...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1058 @bananer is this worth pulling forward if it passes a build? ---
[GitHub] thrift issue #1075: THRIFT-3916 Throw proper errors from JS, not strings
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1075 @bananer is this worth pulling forward if it passes a build? Is it a breaking change? ---
[GitHub] thrift issue #1506: THRIFT-4509: grunt update (rebased)
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1506 With this merged would you say that THRIFT-4509 is now fixed, or is there more work to do? ---
[GitHub] thrift issue #1506: THRIFT-4509: grunt update (rebased)
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1506 I see, you need to build Java first? ---
[GitHub] thrift issue #1505: THRIFT-4513: Fixing java thrift compiler to generate con...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1505 Please squash and rebase on master, I just merged 4 commits that stabilize the CI builds. ---
[GitHub] thrift issue #1502: THRIFT-4509: update grunt to 1.0.2
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1502 In the future you can squash, rebase, and force push to keep the same PR, see: https://thrift.apache.org/docs/HowToContribute ---
[GitHub] thrift issue #1502: THRIFT-4509: update grunt to 1.0.2
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1502 Okay that sounds good; could you squash and rebase on master to prepare for a merge? ---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 Appveyor: It looks like I broke the build yesterday with a check-in that I did not run through CI. I am fixing it. Travis: c_glib, that's new; npm; that might be an intermittent failure Could you squash your PR to one commit to prepare it for merge? ---
[GitHub] thrift issue #1502: THRIFT-4509: update grunt to 1.0.2
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1502 Any special considerations we need to add to the README for js or nodejs if we check in a package-lock.json file? When distributing through npm should this file also be distributed? ---
[GitHub] thrift issue #1486: THRIFT-4337: Able to set keyStore and trustStore as Inpu...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1486 Hmm, looks like I broke the build right before this; I'll make sure this carries through and merge it. Thanks. ---
[GitHub] thrift issue #1497: Do not call workSocket() in TNonblockigServer without en...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1497 Cool so that last test run was without the fix (I thought it had it, but it only had my fixes to TestServer.cpp), and adding your fix seems to have taken care of the header failures somehow: ``` root@1dcb0ce83732:/thrift/src# test/test.py --server cpp --client cpp --regex='.*framed.*-ip.*' Apache Thrift - Integration Test Suite Sun Mar 11 19:39:18 2018 === server-client: protocol: transport: result: cpp-cpp compact framed-ipsuccess cpp-cpp multi-binary framed-ip-sslsuccess cpp-cpp compact framed-ip-sslsuccess cpp-cpp multi-binary framed-ipsuccess cpp-cpp multihframed-ipsuccess cpp-cpp multihframed-ip-sslsuccess cpp-cpp multih-header framed-ip-sslsuccess cpp-cpp multih-header framed-ipsuccess cpp-cpp multij-json framed-ip-sslsuccess cpp-cpp multij-json framed-ipsuccess cpp-cpp json framed-ip-sslsuccess cpp-cpp json framed-ipsuccess cpp-cpp multicframed-ipsuccess cpp-cpp multicframed-ip-sslsuccess cpp-cpp multic-compactframed-ip-sslsuccess cpp-cpp multic-compactframed-ipsuccess cpp-cpp multij-json framed-ip-sslsuccess cpp-cpp multij-json framed-ipsuccess cpp-cpp multijframed-ip-sslsuccess cpp-cpp multijframed-ipsuccess cpp-cpp headerframed-ip-sslsuccess cpp-cpp headerframed-ipsuccess cpp-cpp multic-compactframed-ip-sslsuccess cpp-cpp multic-compactframed-ipsuccess cpp-cpp multi-binary framed-ip-sslsuccess cpp-cpp multi-binary framed-ipsuccess cpp-cpp multi framed-ipsuccess cpp-cpp multi framed-ip-sslsuccess cpp-cpp multih-header framed-ip-sslsuccess cpp-cpp multih-header framed-ipsuccess cpp-cpp binaryframed-ipsuccess cpp-cpp binaryframed-ip-sslsuccess === No unexpected failures. ``` ---
[GitHub] thrift issue #1497: Do not call workSocket() in TNonblockigServer without en...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1497 You need to create a Jira ticket in the THRIFT project (https://issues.apache.org/jira/projects/THRIFT/issues) that describes the bug and resolution so that I can merge it. --- I was able to test these changes out by making some changes to the cpp TestServer for crosstest which I will check in today. I changed my local tests.json file to add "--server-type=nonblocking" to each cpp server and changed some code to make additional cases work: ``` root@8d560c4cc0fa:/thrift/src# test/test.py --server cpp --client cpp --regex='.*framed.*-ip.*' Apache Thrift - Integration Test Suite Sun Mar 11 14:49:02 2018 === server-client: protocol: transport: result: cpp-cpp compact framed-ipsuccess cpp-cpp multi-binary framed-ipsuccess cpp-cpp compact framed-ip-sslsuccess cpp-cpp multihframed-ip-sslsuccess cpp-cpp multihframed-ipsuccess cpp-cpp multi-binary framed-ip-sslsuccess cpp-cpp multih-header framed-ipsuccess cpp-cpp multih-header framed-ip-sslsuccess cpp-cpp multij-json framed-ip-sslsuccess cpp-cpp multij-json framed-ipsuccess cpp-cpp json framed-ipsuccess cpp-cpp json framed-ip-sslsuccess cpp-cpp multicframed-ipsuccess cpp-cpp multicframed-ip-sslsuccess cpp-cpp multic-compactframed-ipsuccess cpp-cpp multic-compactframed-ip-sslsuccess cpp-cpp multij-json framed-ip-sslsuccess cpp-cpp multij-json framed-ipsuccess cpp-cpp headerframed-ip failure(-6) cpp-cpp multijframed-ipsuccess cpp-cpp multijframed-ip-sslsuccess cpp-cpp headerframed-ip-ssl failure(-6) cpp-cpp multic-compactframed-ipsuccess cpp-cpp multic-compactframed-ip-sslsuccess cpp-cpp multi-binary framed-ip-sslsuccess cpp-cpp multi-binary framed-ipsuccess cpp-cpp multi framed-ip-sslsuccess cpp-cpp multi framed-ipsuccess cpp-cpp multih-header framed-ip-sslsuccess cpp-cpp multih-header framed-ipsuccess cpp-cpp binaryframed-ip-sslsuccess cpp-cpp binaryframed-ipsuccess === ``` (I think you can ignore the header failures) ---
[GitHub] thrift issue #1497: Do not call workSocket() in TNonblockigServer without en...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1497 What's the Jira ticket ID for this work? ---
[GitHub] thrift issue #1497: Do not call workSocket() in TNonblockigServer without en...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1497 Wow, haven't seen a green build in a while. Of course, none of the cross tests exercise nonblocking code. :( Would be nice to add this as another matrix option. ---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1448 I tried to use this work and did a little bit of cmake work to straighten out options. What I found is that there is a boost dependency in the safe_numeric_cast in TTransportException.hpp and there's another boost dependency in TProtocol.h (for endian detection) that needs to be resolved. What I would suggest is that you try building on Windows without boost, disabling build of tests and tutorials, so that all you build is the compiler and the C++ library. You can see what I did in my referenced pull request in my fork. ---
[GitHub] thrift pull request #1448: Thrift-4441: Support compilation without Boost
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1448#discussion_r173620082 --- Diff: build/cmake/DefineOptions.cmake --- @@ -91,11 +91,26 @@ if(WITH_CPP) option(WITH_STDTHREADS "Build with C++ std::thread support" OFF) CMAKE_DEPENDENT_OPTION(WITH_BOOSTTHREADS "Build with Boost threads support" OFF "NOT WITH_STDTHREADS;Boost_FOUND" OFF) + +set(WITH_CPP_SUPPORT OFF) --- End diff -- I think we would be better off testing for C++11 features and ensuring that if we find what we need, we can enable certain things. For example if we ask cmake to check for support of cxx_defaulted_functions then we can enable the boost-less noncopyable. One can typically check for cxx_nullptr in order to determine if there is smart pointer support, or write a custom check for std::shared_ptr detection. Relying on checking compiler versions is error-prone and not as portable. This is a good start to optionally eliminating boost from the C++ runtime library. I can work on the feature checks as an extension to this work. ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 @jfarrell can you please give a final say on this? ---
[GitHub] thrift issue #1497: Do not call workSocket() in TNonblockigServer without en...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1497 Rebase on master and let's see what happens, thanks. ---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 The current master is fairly clean except for a recurring dlang issue. Squash and rebase on master. Squash to one commit please. ---
[GitHub] thrift issue #1479: THRIFT-4474: generate PHP code use PSR-4 style default
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1479 Document all breaking changes in the language README. ---
[GitHub] thrift issue #1480: [nodejs] Fix issue with connection failures silently fai...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1480 This needs to be retargeted at the master branch and it needs an Apache Jira thrift ticket. See https://thrift.apache.org/docs/HowToContribute ---
[GitHub] thrift issue #1494: THRIFT-4495: Allow `undefined` for non-required Erlang r...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1494 Ugh, build failure is dlang related, not to this. ---
[GitHub] thrift issue #1494: THRIFT-4495: Allow `undefined` for non-required Erlang r...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1494 I'm running this and 4497 through local tests since it wasn't based on good CI builds. ---
[GitHub] thrift issue #1504: Fix wrong @param in comment
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1504 Usually we ask for a Jira ticket for each change but I'm not going to considering this is a small doc change. In the future please see: https://thrift.apache.org/docs/HowToContribute ---
[GitHub] thrift issue #1500: THRIFT-4509: use nodejs 8.x from nodesource.com in travi...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1500 As of yesterday afternoon 4.x is no longer used. Due to the dependencies, and due to the fact that node.js 4.x LTS ends next month, I moved the "oldest" make check job to use nodejs 6.x (ubuntu-xenial) and the current one uses 8.x (ubuntu-artful) - this is the one that runs make check, make cross, ubsan, cppcheck, etc. We still need to modernize the code/test for js and nodejs however. ---
[GitHub] thrift pull request #1497: Do not call workSocket() in TNonblockigServer wit...
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1497#discussion_r172695014
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -472,6 +472,18 @@ void TNonblockingServer::TConnection::workSocket() {
}
// size known; now get the rest of the frame
transition();
+
+// If the socket has more data than the frame header, continue to work
on it. This is not strictly necessary for
+// regular sockets, because if there is more data, libevent will fire
the event handler registered for read
+// readiness, which will in turn call workSocket(). However, some
socket types (such as TSSLSocket) may have the
+// data sitting in their internal buffers and from libevent's
perspective, there is no further data available. In
+// that case, not having this workSocket() call here would result in a
hang as we will never get to work the socket,
+// despite having more data.
+if (tSocket_->hasPendingDataToRead())
--- End diff --
I am concerned this can cause an infinite loop if data is always available,
since it calls itself.
---
[GitHub] thrift pull request #1497: Do not call workSocket() in TNonblockigServer wit...
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1497#discussion_r172694884
--- Diff: lib/cpp/src/thrift/server/TNonblockingServer.cpp ---
@@ -472,6 +472,18 @@ void TNonblockingServer::TConnection::workSocket() {
}
// size known; now get the rest of the frame
transition();
+
+// If the socket has more data than the frame header, continue to work
on it. This is not strictly necessary for
+// regular sockets, because if there is more data, libevent will fire
the event handler registered for read
+// readiness, which will in turn call workSocket(). However, some
socket types (such as TSSLSocket) may have the
+// data sitting in their internal buffers and from libevent's
perspective, there is no further data available. In
+// that case, not having this workSocket() call here would result in a
hang as we will never get to work the socket,
+// despite having more data.
+if (tSocket_->hasPendingDataToRead())
--- End diff --
Should this be an "if" or a "while"?
---
[GitHub] thrift issue #1495: THRIFT-4497: Use map() field type for Erlang type for ma...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1495 Hi, please rebase this on the current master and force push to kick a new build - I think the CI build environment is stable again. ---
[GitHub] thrift issue #1494: THRIFT-4495: Allow `undefined` for non-required Erlang r...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1494 Hi, please rebase this on the current master and force push to kick a new build - I think the CI build environment is stable again. ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 If you haven't seen it I recommend reading `build/docker/README.md` and use the `ubuntu-artful` image to run a cross test with `build/docker/scripts/cross-test.sh` inside the docker container. Doing this effectively runs the same build as travis would (job #1 which is the cross test). You can run only the nodejs tests with domain sockets by running the command `test/test.py --regex '.*nodejs.*domain.*'` after completing cross-test.sh. ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 The presence of "--domain-socket" to the client or server implies uds. This needs to be fixed, and domain needs to be moved from haskell to nodejs, and the cross tests that are failing (many) in the ubuntu-artful docker image need to be investigated. For example it is likely expected for the "http-domain" subset of tests to fail given the http-ip ones fail already, however in one test I found that the call to fs.unlinkSync(path) [server.js] failed because the domain socket doesn't exist before the test begins, and this call throws an exception, causing it to fail. ---
[GitHub] thrift pull request #1491: THRIFT-4489: Add UDS support for nodejs thrift cl...
Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1491#discussion_r172557523 --- Diff: test/tests.json --- @@ -221,7 +221,8 @@ "framed" ], "sockets": [ - "ip" + "ip", + "domain" --- End diff -- You added domain tests to the haskell group, not the nodejs group. ---
[GitHub] thrift issue #1496: THRIFT-4476: Typecasting problem on list items (+ low pr...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1496 Sourceforge was down for a while. I am working on getting the builds back to green (passing). I recommend that you keep your pull request squashed to one commit. In fact since you are doing iterative development you are clogging up the pull request build pipeline. I recommend you close this pull request and work on your own fork until you are satisfied you have resolved the issues. I am hoping to have the builds back to "green" by the end of today. ---
[GitHub] thrift issue #1269: THRIFT-4187 Allow dart framed transport to read incomple...
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1269 This needs to be rebased and completed. ---
[GitHub] thrift issue #1380: Support x64 build mode on Windows with DMD v2.076.0.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1380 The tutorials didn't get updated and as a result fail to build in the autotools build (build/docket/script/autotools.sh) which is what some of the Travis CI build jobs do. I am attempting to fix this. ---
[GitHub] thrift issue #1448: Thrift-4441: Support compilation without Boost
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1448 Thanks, that's incredibly helpful when it comes to merging it. ---
[GitHub] thrift issue #1491: THRIFT-4489: Add UDS support for nodejs thrift client
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1491 Where does the "--type uds" option get passed into the python test client or server in order to convince it to use domain sockets? In other test suites like cpp, only "--domain_socket" is required in order to force it to use "domain". ---
[GitHub] thrift issue #1499: Fix python build on Vagrant Windows boxes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1499 THRIFT-4505 ---
[GitHub] thrift issue #1499: Fix python build on Vagrant Windows boxes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1499 Hi, in the future please follow the instructions at https://thrift.apache.org/docs/HowToContribute, particularly opening a Jira ticket and tracking the pull request against it. I will do this for you this time. Thanks for the submission. ---
[GitHub] thrift issue #1380: Support x64 build mode on Windows with DMD v2.076.0.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1380 Opened THRIFT-4504 to track this. ---
[GitHub] thrift issue #1380: Support x64 build mode on Windows with DMD v2.076.0.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1380 I'd like to see this squashed to one commit and rebased one more time; our CI builds are not in good shape but I can run it on my local system to prove it out and they deal with CI issues when that system is back and operating (sourceforge has been down for days and we depend on some components hosted there). ---
[GitHub] thrift issue #1474: Recognize fbthrift TApplicationException codes
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1474 Based on my read of the file from which the original source information came, the original fbthrift project files are Apache Source licensed. So I'm not certain we need to do anything in particular, but @jfarrell said that we do. As such, I am going to wait until he gives the okay. ---
[GitHub] thrift issue #1412: [THRIFT-82] Add Common Lisp support
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1412 I updated the ubuntu-artful image to SBCL 1.4.4 and it seems to be stable. Merging. ---
