Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
Thanks for all your help @hanm! I added a PR for the `branch-3.4` port too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
@andschwa This patch just landed on master. Thanks for your contribution.
I'll review the branch-3.5 port and commit it later this week if it looks good.
---
If your project is set up for it, you ca
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
Thanks for the extra efforts on integration testing, @andschwa. I'll commit
this tomorrow.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as w
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
I managed to get the Mesos unit tests for ZooKeeper ported to our CMake
system, which much more thoroughly exercises the C client. I've integrated this
patch and the CMake system with the Mesos b
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
looks good. test failures are irrelevant (we should really get the flaky
tests fixed soon.).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
This
[here](https://github.com/apache/zookeeper/pull/306/files#diff-1ea258ac6b9efc24b67d2c2a704cfe5fR145)
is all I changed; it ensures that the `config.h` file is always put into the
source `inc
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
@hanm I'm likewise giving this some more testing too. I'm integrating the
patch into Mesos on the Linux side (where we were building with Autotools), and
porting our ZooKeeper-explicit unit tests
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
Thanks for update. LGTM. Will merge by end of this week. Give a few more
days in case someone has additional feedback for this.
---
If your project is set up for it, you can reply to this email and
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
@hanm I'm going to let you take care of the title, maybe it should be:
> ZOOKEEPER-2756 and ZOOKEEPER-2841: Improve cross-platform support with
CMake and refactor
But I don't kno
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
@hanm ah, I see! That is no problem. I have taken the two messages,
improved them a bit as one message, and updated the description. It will be
easier to backport it this way.
---
If your proje
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
If we want two commits, then we need two pull requests. Two (or more )
commits appertain to a single pull request will be squashed to a single commit
at commit time by our commit script. I think it's
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
> Can you please describe what kinds of test / integration test you did on
windows?
Unfortunately, we don't have the unit tests building on Windows (under any
tools). So I ensured I coul
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
@andschwa
Patch looks good to me. The readme update looks good too.
I also verified Linux and Mac c client builds with the patch. Unfortunately
I don't have a windows box to test. Can y
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
Hey, yay, Jenkins passed!
---
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 wi
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
@hanm I ended up just adding it [straight to the
src/c/README](https://github.com/apache/zookeeper/pull/306/files#diff-a722fe5ba032cb8da6c78d0e929f4ac5R74),
let me know if you think it should hav
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
@hanm ah, documentation for it would indeed be good! Can do. Anything else
you need for this? If not, I'll get the readme fixed up Monday morning and ping
you afterward.
---
If your project is
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
@andschwa Thanks for clarification regarding the coexisting of CMake and
makefile. I agree and let's keep the current folder structure, but I think some
documentations would be good to help clarify t
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
Just to give you an idea of what we currently have to do in order to use
the ZooKeeper C Client in Mesos, we have
[this](https://github.com/apache/mesos/blob/b4f8d1f588841b0a7395c24630963be5620db
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
@hanm the PR is rebased, retested, and has some extra notes in it now for
some weird CMake things.
I would not put the CMake files in a separate folder. There are couple of
reasons for t
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
Should we put the CMake files in a dedicated folder called "windows" or
something like that? Otherwise we will have the CMakeList file, and the
existing makefile and config files (which are not touch
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
@hanm Awesome! The patches seem to be working well, and I think we don't
need to worry about maintaining compatibility with, say, Visual Studio 2008, in
new versions of the ZooKeeper client. Old
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/306
>> Is this failing test my fault? It's... odd.
No it's not your fault. This is a known flaky test, and I will have a patch
for this soon.
>> I'm not sure how you guys like to do depe
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
@hanm I'm not sure how you guys like to do dependent patches.
ZOOKEEPER-2756 and ZOOKEEPER-2841 should both be taken to resolve problems
using the C client in other projects (e.g. Mesos ð), an
Github user andschwa commented on the issue:
https://github.com/apache/zookeeper/pull/306
Is this [failing
test](https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/870/testReport/org.apache.zookeeper.server.quorum/ReconfigDuringLeaderSyncTest/testDuringLeaderSync/)
my f
24 matches
Mail list logo
