You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by allengeorge <gi...@git.apache.org> on 2017/05/01 11:13:37 UTC
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
GitHub user allengeorge opened a pull request:
https://github.com/apache/thrift/pull/1260
THRIFT-4186 Add travis build for Rust
Client: rs
* Install Rust in ubuntu/centos docker images
* Fix type bounds on TMultiplexedProcessor
* Add multiplexing support to cross-test server and client
* Run multiplexed tests in rs cross-test
* Temporarily add dart->rs framed as "known failures"
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/allengeorge/thrift thrift-4186
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1260.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 #1260
----
commit b1f701a112768b29ede23a1e3d109933b4750965
Author: Allen George <al...@gmail.com>
Date: 2017-04-28T14:22:03Z
THRIFT-4186 Add travis build for Rust
Client: rs
* Install Rust in ubuntu/centos docker images
* Fix bounds on TMultiplexedProcessor
* Add multiplexing support to cross-test server and client
* Run multiplexed tests in rs cross-test
* Temporarily add dart->rs framed "known failure"
----
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
No problem - I can change it to do that. Should that be in a different commit/pull-request, or merged in 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 project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1260
Whomever does commit, there's a comment at the top of the docker files for ubuntu and centos that need a line removed - now the only language not represented in the images is netcore.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
@Jens-G Thank you!
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1260#discussion_r114321962
--- Diff: test/rs/src/bin/test_server.rs ---
@@ -104,35 +104,43 @@ fn run() -> thrift::Result<()> {
}
};
- let processor = ThriftTestSyncProcessor::new(ThriftTestSyncHandlerImpl {});
+ let test_processor = ThriftTestSyncProcessor::new(ThriftTestSyncHandlerImpl {});
- let mut server = match &*server_type {
- "simple" => {
- TServer::new(
+ match &*server_type {
+ "simple" | "thread-pool" => {
+ let mut server = TServer::new(
i_transport_factory,
i_protocol_factory,
o_transport_factory,
o_protocol_factory,
- processor,
+ test_processor,
1,
- )
+ );
+
+ server.listen(&listen_address)
}
- "thread-pool" => {
- TServer::new(
+ t if t.to_owned().starts_with("multi") => {
+ let second_service_processor = SecondServiceSyncProcessor::new(SecondServiceSyncHandlerImpl {},);
+
+ let mut multiplexed_processor = TMultiplexedProcessor::new();
+ multiplexed_processor.register("ThriftTest", Box::new(test_processor));
+ multiplexed_processor.register("SecondService", Box::new(second_service_processor));
--- End diff --
So nice to see more language support for cross test like this!
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
@jeking3 Oh. Totally didn't realize that - thanks for clarifying! I've updated my `TMultiplexedProcessor` implementation to have a backwards-compatibility mode, and am re-running the build right now.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
@jeking3 Happy to make the change to the travis jobs! Let me modify this PR with the defect change and then work on rebalancing the travis jobs.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
Please pull the latest commit. I updated to fix a bug in my test server.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
@jeking3 I've updated the travis build with rust support, but I think I'm running into timeouts because now the builds take too long :/ Any ideas what I should do?
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1260
Keeping it separate is fine. Thanks for offering to make that change to the travis jobs.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
I had to split up binary/compact protocols because the builds were timing out with them together. And, AFAICT, the unit tests aren't running for builds 1 and 2.
Also (I didn't change this) but I'm unsure of the utility of some of the builds in the matrix:
- TEST_NAME="C C++ C# D Erlang Haxe Go (automake)"
- TEST_NAME="C C++ Plugin Haskell Perl - GCC (automake)"
- TEST_NAME="Java Lua PHP Ruby Dart Node.js Python (automake)"
Looks like these are actually running the unit tests? If so, then why is C/C++ repeated?
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
@Jens-G @jeking3 Can I get any guidance on how the clients/server get their arguments? I'm confident it's not a bug in the rust server. I can invoke the c_glib client and rust server manually on the command line and they both communicate properly. Somehow the c_glib client gets `binary` while the rust server gets `multi` (and the corresponding arguments for compact).
I looked into `crossrunner/collect.py` but the code is a little tough to decipher, and it's not clear how it's determining an intersection.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1260
I believe that is intentional for backwards compatibility. A multiplexing server should be able to accept an older client and use the "default" service for it - at least in the Java multiplexing server. So they should end up using the same protocol (binary or compact) but a multi-binary server should be able to service a binary client.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
OK. I really don't understand `tests.json`. I'm confident that my code is doing the right thing - it's just that the server is invoked in multiplexed mode and the client is invoked in non-multiplexed mode. Super bizarre.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1260#discussion_r114529981
--- Diff: test/rs/src/bin/test_server.rs ---
@@ -104,35 +104,43 @@ fn run() -> thrift::Result<()> {
}
};
- let processor = ThriftTestSyncProcessor::new(ThriftTestSyncHandlerImpl {});
+ let test_processor = ThriftTestSyncProcessor::new(ThriftTestSyncHandlerImpl {});
- let mut server = match &*server_type {
- "simple" => {
- TServer::new(
+ match &*server_type {
+ "simple" | "thread-pool" => {
+ let mut server = TServer::new(
i_transport_factory,
i_protocol_factory,
o_transport_factory,
o_protocol_factory,
- processor,
+ test_processor,
1,
- )
+ );
+
+ server.listen(&listen_address)
}
- "thread-pool" => {
- TServer::new(
+ t if t.to_owned().starts_with("multi") => {
+ let second_service_processor = SecondServiceSyncProcessor::new(SecondServiceSyncHandlerImpl {},);
+
+ let mut multiplexed_processor = TMultiplexedProcessor::new();
+ multiplexed_processor.register("ThriftTest", Box::new(test_processor));
+ multiplexed_processor.register("SecondService", Box::new(second_service_processor));
--- End diff --
Glad to add it :)
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
I've rebalanced the matrix. Trying again.
@jeking3 Having a weird problem with the c_glib client. It's using the "binary" or "compact" protocol only when invoked in multi/multic mode.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
@jeking3 Thanks for pointing me in the right direction. Everything now passes!
I realize you're not committing, but would it be possible for one of the other maintainers - @Jens-G perhaps? - to get this change in? To summarize:
* Added rust cross-test and unit-test support to travis
* Added rust multiplexed client/server cross-tests; fixed some bugs in my multiplexing impl.
* Split up test matrix to avoid timeouts
* Discovered bug in Dart framed transport implementation; filed [THRIFT-4187](https://issues.apache.org/jira/browse/THRIFT-4187)
So, with the exception of the two dart tests, every other rust cross-test and all unit tests pass. I've also fully linted and formatted the rust code. Thank you all!
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
@jeking3 One hopes that...congratulations are in order? :)
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1260#discussion_r114546199
--- Diff: test/known_failures_Linux.json ---
@@ -224,5 +224,7 @@
"hs-py3_json_framed-ip",
"java-d_compact_buffered-ip",
"java-d_compact_buffered-ip-ssl",
- "java-d_compact_framed-ip"
+ "java-d_compact_framed-ip",
+ "rs-dart_binary_framed-ip",
--- End diff --
Oh. If you're talking about a JIRA, I've already filed one: [THRIFT-4187](https://issues.apache.org/jira/browse/THRIFT-4187)
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1260
Essentially same job as before, we were acquired and cut over on May 1. Thanks are always appreciated in any form though! :)
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1260
I can't do any merges at the moment due to a change in employment, and I need to get any legal hurdles out of the way, but one of the other committers should be able to - assuming the new build passes. :)
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1260
One is clang and one is gcc; I recently lowered us from 25 to 19 build jobs and got rid of the ones we didn't need or were redundant.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1260#discussion_r114530042
--- Diff: test/known_failures_Linux.json ---
@@ -224,5 +224,7 @@
"hs-py3_json_framed-ip",
"java-d_compact_buffered-ip",
"java-d_compact_buffered-ip-ssl",
- "java-d_compact_framed-ip"
+ "java-d_compact_framed-ip",
+ "rs-dart_binary_framed-ip",
--- End diff --
OK. Will do. Didn't realize there was a defect list.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1260
We are definitely getting close to the limit. There are plenty of things we can do. We could change Build job #1 to build everything and run all tests (which it does), and test headers, multiplexed, and json transport. We could change build job #2 to build everything, NOT run unit tests (redundant to job #1), and test binary and compact protocols. This would probably even things out a bit and add some breathing room for netcore addition to crosstest.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1260#discussion_r114321825
--- Diff: test/known_failures_Linux.json ---
@@ -224,5 +224,7 @@
"hs-py3_json_framed-ip",
"java-d_compact_buffered-ip",
"java-d_compact_buffered-ip-ssl",
- "java-d_compact_framed-ip"
+ "java-d_compact_framed-ip",
+ "rs-dart_binary_framed-ip",
--- End diff --
We should file a defect on the poor dart behavior that you described.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request #1260: THRIFT-4186 Add travis build for Rust
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1260
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
Also, I'm fixing the CentOS issues. I mistakenly assumed that `sh` would be the same in both ubuntu and Centos. Guess I was wrong...
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #1260: THRIFT-4186 Add travis build for Rust
Posted by allengeorge <gi...@git.apache.org>.
Github user allengeorge commented on the issue:
https://github.com/apache/thrift/pull/1260
Ah. Gotcha. Sorry - I should have realized that. I'll pay closer attention next time.
---
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 enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---