[GitHub] thrift issue #1191: Thrift 3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 What testing have you done to verify this code (I have to ask because there is no multiplexed protocol testing being done in crosstest currently)? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: Thrift 3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 In the future please format your pull request title to start with THRIFT-3706 so that the bots link the pull request to the open Jira item. Thanks! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: Thrift 3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 Hi Jeking3, You are right! I forgot this time to do the proper naming of the merge request. Yesterday I was thinking about the cross test. I will try to add a cross test for Java vs c_glib since it's the one actually tested in our own internal test. Do you have a guideline for the cross test? As you know I'm little lost there. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 @jeking3 Please explain how this can happen. Just did a rebase and merged my changes that only adds few files (I think two or three). If master is so controlled that no build failures can go in (you where very strict with my tests)... How can it give a cross test failure? This is not my fault. How others can be merging changes that break the build? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 I added some cross tests against Java. Maybe others can add the rest when the protocol is ready for the other languages. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 What do I have to do to be a contributor? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 I'm a committer on the project which means I have write access to the apache repository for the project. Most changes come in like this, and one of the committers need to merge it into master. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 I must say that like the SSL patch this is already tested in production. Java vs c_glib. We have to add support for the Server c_glib and Client Java. But I suppose this time it will work without problems. About the multiplex fact, you cannot change the multplexor in runtime while it's already running. You must do it at initialization time. I want to add in the future the ability to add new multiplexed services "on the fly". But I should be add the properties in gobject and make sure I don't break multithreading. So this has to be added at latter step, like always. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 I added "multiplexed" support to make cross in Travis CI if you rebase on master. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 Hi Jeking3, Why don't you merge it and add a new issue for others to add multiplexing protocol? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 I'm not going to merge it until it passes make cross with multiplexed protocol. There's nothing in the pull request and the build system that proves it works and will keep working. I am working on THRIFT-4084 right now, I will see if I can get to it soon if you do not. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 Did you see that the test is working against Java? If you try a connection with and without multiplexed in one of the ends the connection doesn't work. The only way to see if the data is sent correctly is by using wireshark and see how the prefix is automatically added to the service. And processed in the server end. What do you need to merge? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 Discussing with folks who created crosstest what we should do. My preference is to have a single "multiplexed" protocol test that uses the binary protocol. That will be the most widely used combo, and layering multiplexed on top of any other protocol does not test any new functionality of multiplexed; but it might catch strange interactions that we could miss if we don't test all of them. I'm not fond of the explosion of the test matrix, doubling all the tests potentially. If you look in tests.json you will see protocols like "binary" and then "binary:accel", also "compact" and "compact:accelc". This naming format basically says that if crosstest is testing "binary" it will match "binary" and "binary:accel", however the test wll be run once with a --protocol of "binary" and once with a --protocol of "accel". I didn't want to add "binary:multi", "compact:multic", "json:multij", "header:multih" but that would be the logical extension of the pattern that already exists in crosstest. So I'm working on a copy of your branch right now to simplify down to "multiplexed" and then I want to see it run in the crosstest locally. Then once I hear back from some folks we can move forward if they are okay with the simplification. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 Well I was hoping the multiplexed implementation was compatible with older clients using binary protocol but it is not. I'm going to send you a PR that changes the Java Server and the c_glib client to work with "multiplexed" as a protocol; we will use binary protocol under all the multiplexed protocol tests. I added support in the java test server to have a second registered processor. It would be nice to augment c_glib to test against "Second" as well as "ThriftTest" so that we're actually testing the multiplexing part. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 @gadLinux for some reason I cannot send you a PR for this one like I had before, possibly because I rebased on master. If you cherry-pick b2e4e1464b74733ccf78afd68abbe94b92aec96a from my repository it will give you the changes you should need. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 I cannot do it since this commit is in thrift main outside master. So I cannot see it. You cannot merge because this branch is still not merged so it's still in my fork. You can merge and then apply the patch you want. Otherwise the only way I think is to make a patch. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1191 Weird, I'm not exactly sure what happened here, but I submitted a new PR rebased against master incorporating your changes here and I added one more commit - see PR #1199. If that passes I'll squash my commit into yours down to a single commit so you show up as the author of the pull request into the master when I merge it in. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1191: THRIFT-3706 - Implement multiplexor.
Github user gadLinux commented on the issue: https://github.com/apache/thrift/pull/1191 Jus great, thank you! ---
