> On Jan. 27, 2016, 3 a.m., Alex Clemmer wrote:
> > Aside from the relatively minor comments below, I have one major 
> > suggestion: I'd like to point Windows builds at ZK commit 
> > 06d3f3fa1bff258e62c0670309ad1849b1434bb1[1], (or _some_ commit later in the 
> > version history) so that we can transition as much away from patching the 
> > ASM and .c files as we can. I'll volunteer to prepare a candidate fix for 
> > this patch tonight so we have something to complete the review tomorrow.
> > 
> > My justification follows:
> > 
> > (1) At some point, when the diff between the C/ASM source in the supported 
> > version and the version that compiles on Windows becomes big enough, it 
> > stops being worth it to maintain a .patch file instead of upgrading the ZK 
> > version. In this particular case, I am not comfortable replacing that ASM 
> > code with that function call without a code review from the ZK team, and 
> > since there is a competing implementation in the master branch (see issue 
> > in the tracker[2] and the `fetch_and_add` function in the relevant source 
> > file[3]) I propose we just upgrade the Windows build to point at a version 
> > of ZK with this patch.
> > (2) We already point to commit hashes for other third party projects, like 
> > glog, so this is not a solution without precedent.
> > 
> > My next steps are the following:
> > 
> > * We'll probably need to keep the patch file, since we still need .vcxproj 
> > and .sln files that build on VS2015.
> > * The current build solution basically unpacks a tarball from the 
> > `3rdparty` directory that has the Jute files pre-generated; we'll need to 
> > make another tarball that the Windows can download that also has the jute 
> > files generated.
> > 
> > Let me know of other thoughts or issues; otherwise I'll see you on the 
> > other side with a (god willing) working version of this patch.
> > 
> > [1] 
> > https://github.com/apache/zookeeper/commit/06d3f3fa1bff258e62c0670309ad1849b1434bb1
> > [2] https://issues.apache.org/jira/browse/ZOOKEEPER-1635
> > [3] https://github.com/apache/zookeeper/blob/trunk/src/c/src/mt_adaptor.c

I happy to say that we have a working cadidate fix for these changes. They're 
in two temp commits: 
https://github.com/hausdorff/mesos/commit/3ed2626c11ddf45108d2a9711131cce80bd85dfb
 
https://github.com/hausdorff/mesos/commit/945b239de3af4f4a9fc1b36c88d70f9913f1cda1
 I'll work with Lawindi tomorrow to formulate them into cohesive patches as 
part of these two ZK patches he's made.


- Alex


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40851/#review116500
-----------------------------------------------------------


On Jan. 26, 2016, 9:03 p.m., M Lawindi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40851/
> -----------------------------------------------------------
> 
> (Updated Jan. 26, 2016, 9:03 p.m.)
> 
> 
> Review request for Alex Naparu, Dario Bazan, Daniel Pravat, Alex Clemmer, and 
> M Lawindi.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build.
> 
> 
> Diffs
> -----
> 
>   3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da 
> 
> Diff: https://reviews.apache.org/r/40851/diff/
> 
> 
> Testing
> -------
> 
> - Applied patch to original zookeeper 3.4.5
> - Successfully opened 2015 solution in VS2015
> - Cli and Zookeeper build successfully
> 
> 
> Thanks,
> 
> M Lawindi
> 
>

Reply via email to