Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Hi @jeking3 it was a hard feature to merge... Hope we will do finer next
time. Thank you for your continued support.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
@gadLinux you can close this PR if you want; I will carry it forward in a
combined PR for our stuff: https://github.com/apache/thrift/pull/1414
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
I'm going to carry your changes into another PR that has cpp client and
server multiplexed and java multiplexed testing added, and fixes some other
bugs I found along the way. Once it builds clean
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
I tried to debug the Rust thing. But it seems is not receiving the response
from c_glib testNest in my case. I debugged the server and the response is
sent, and flushed. So I don't catch why RS is
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Just curious that The error messages I get are different. In fact they got
blocked on the RS (Rust?) client, at least it seems to.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Under THRIFT-2013 I am adding multiplexed cpp testing to the suite. Will
see how it interacts. If only "rs" client is misbehaving, we can disable.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
I cannot merge in a change that will knowingly break CI builds.
The tests that are failing need to be fixed (preferably) or disabled using
the "test.py -U merge" I (hopefully) documented
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
But I don't know about the languages that are failing. This is why I told
you to open other issue and merge this.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
The tests that failed do not use ssl.
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Can it be because the SSL server. That's not implemented on c_glib?
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
I cannot merge anything while the CI tests are failing due to language
specific issues.
```
===
*** Following
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Hi @jeking3 can you merge please?
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Hi @jeking3 I think at the end it was part of the test client... What do
you think? It worth it merge it?
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
In fact @jeking3 Some of the failures are not related to this issue. So
please don't ask me to fix them as part of this issue. One of them is caused in
the Java Server because the exception
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
What I'm telling you is that I need help if you want to fix everything on
this language. I don't have all the time to implement the full stack. This is
an implementation of the processor, not all
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
@jeking3 I don't think is an error, but that all this cross tests are
directly not supported. No all protocols are implemented.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
@gadLinux I think you need to re-read the error messages. It says
GLib-WARNING and GError. That's the c_glib library. Further it looks like all
the "multi" protocol tests failed with c_glib on
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Yes but this seems to be in other languages that are not in my scope. So it
should be others that fix them. So I think it's good enough for now. Can you
merge it? And maybe open a new bug to trac
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Job 6 failed due to https://issues.apache.org/jira/browse/THRIFT-2913 which
pops up far too often.
Job 4 failed legitimitely and indicates there is still something that needs
to be fixed:
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
4197.6 Fails... I'm done. Don't know what else to do.
Please check what is it...
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
@jeking3 It doesn't fail for me, even in the docker container. Please
review and merge.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Try "dockerbuild ubuntu-xenial" then "dockerrun ubuntu-xenial", then
"build/docker/scripts/autotools.sh" inside the docker environment as that is
how travis runs, you will have the exact same
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Ok I was unable to run as you said in travis. But the errors the code was
giving was because somewhere in build system is configured to compile some
binaries as C90. The errors where mostly
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
With the new image build/docker/scripts/autotools.sh ran ok.
I'm running the other... But no error sofar...
I will try to add -pendantic to the compilation if it doesn't have it in my
host
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Unfortunately it doesn't work for me on the second command.
dockerrun ubuntu-xenial
Unable to find image 'ubuntu-xenial:latest' locally
docker: Error response from daemon: repository
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
If I merge this, it will break all future builds.
Errors in c_glib start at:
https://travis-ci.org/apache/thrift/jobs/300505547 line 2990
if you run the docket ubuntu-xenial image
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
@jeking3 I don't know why some compilations fails. Maybe you could check
because I don't know about what travis is doing. For me seems ok.
Our tests are now ok. I added some fixes to the tests.
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Please merge it or give me a reason to not do it.
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
I finally removed the binary protocol from the list of the limits feature
tests. I don't know if I did right but it seems so. This bug also contains
important bugfixes for the connection
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Rebased and lots of fixes added. Crossing fingers.
I should opened a new issue but since all bug fixing were discovered while
doing this incident I took the opportunity to fix everything here.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
As far as I know builds in CI are stable. Rebase against master.
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
@jeking3 Do you think it would be hard to resolve? If you need me to do
something. Can you point me the right direction. Remember to remove the binary
protocols from the cross tests since they
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Someone broke the build system or there's a patch that made things worse.
Anyway. Check tests manually. I think it's ready to be merged. In fact what I
solved in last commit was not related to my
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
@jeking3 It will fail because binary protocols doesn't use limit string
parameter. Can you remove them from tests since it's not supported.
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Yes, there are a couple of them on the wild. I will take a look ASAP. I had
three of my servers broke at the same time this week and I'm was crazy until
today to bring them up again. Give me a
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Seems like it still has some issues in the cross test with c_glib.
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Ok. Fixed a bug in the test client. And added the default protocol for the
multiplexed. That way the first registered protocol becomes default. This
allows you to use the multi without much
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
What I'm thinking looking at Java implementation is that on c_glib we can
set a property in the processor to define what's the default handler to be get
in case of a not multi client. Let me see
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
@jeking3 I don't think binary and multi server can be compatible. In fact I
see it failing everytime and this is why I didn't set your protocol
recommendations.
Let me explain. When a
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
multic is for compact and multi is for binary
they are backwards compatible so one can use a binary client against a
multi server (which is why you will see servers with "binary:multi"
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
What's multic and multi. What's the difference?
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Now seems ok. This is another big change. So I will try to do cross tests
after. Meanwhile I cross validated with Java. Already in production with this
stuff...
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Hmm, I ran cross tests locally and multi isn't being exercised for c_glib
server:
```
root@ecb350ad7c01:/thrift/src# test/test.py --server c_glib --client java
Apache Thrift -
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
This passed for me locally on a full cross, so I'll merge it.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
@gadLinux I am running the cross test suite locally. The errors in the
build appear to be in c_glib...
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Perfect, now fails on things not related to my changes.
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
It seems the protocol decorator was removed, I suppose I copied and pasted
wrong...
I always forget that the build system is Cmake and forget to test it
against it.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Ruby error in build job 4 is
https://issues.apache.org/jira/browse/THRIFT-2913. c_glib unit tests ran so we
can overlook this failure.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Build job 4 failed in ruby testing. Looking for a match in PRs or Jira.
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
I just went back in history and pushed again after rebase. Now it should be
ok
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Try this:
```
git checkout master
git pull upstream master
git checkout -b THRIFT-4329-take-2
git cherry-pick 1d1d9d8
git cherry-pick 0aedc8a
git cherry-pick 3df26ec
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Doesn't look like a rebase, unfortunately.
What I would suggest is that you refresh your master from upstream, branch
off it, cherry-pick the original commits you made in order, then squash them
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
rebased
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
Ok. I will do it.
---
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1361
Now that the build issues seem to be out of the way, I've merged the
previous 3 PRs from you and I'd ask that you squash and rebase this on the
current master and submit it again. I'd like to see
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
@jeking3 Can you merge this, please?
---
Github user gadLinux commented on the issue:
https://github.com/apache/thrift/pull/1361
@jeking3 a memory leak is getting me crazy maybe we can check together. In
the multiplexed protocol, line 169 we should release that protocol but if I do
it the code seems to fail with memory dump
57 matches
Mail list logo
