> On April 16, 2014, 3:22 p.m., Benjamin Hindman wrote: > > This is awesome Tobi! > > > > One question: Can we actually still use protobuf.jar with the example > > frameworks? My expectation is that existing frameworks should be able to > > continue to use their protobuf dependencies and we've just shaded all use > > of protobuf from them. That way we don't require frameworks/users to use > > org.apache.mesos.com.google.protobuf... but perhaps I'm missing something > > here? Assuming that works, we can drop all the changes to Java example code > > (which should make this diff even simpler) since that will stand as a > > reference for how we expect people to still use Mesos. > > > > Also, it's great to see the configure check and apparently our build > > machines don't have maven!? I'll look into that for you ASAP. > > Tobi Knaup wrote: > Thanks Ben. In the current form you have to get rid of the protobuf jar > because the code generated from the Mesos protos expects the shaded > namespace. For example ExecutorInfo.Builder#setData() expects a > org.apache.mesos.com.google.protobuf.ByteString now. > > This is awkward and I thought about a better solution. We should build > two jars: The regular mesos-VERSION.jar will be unchaged, and we build an > additional shaded mesos-with-protobuf-VERSION.jar for projects with > conflicting protobuf versions. That way the "normal" way remains the same, > but we give affected projects an upgrade path at the small cost of changing > the namespace. Over time these projects will hopefully move to 2.5 and can > use the regular jar again. > > Thoughts? > > Benjamin Hindman wrote: > Ah, I see now. The two jar solution sounds good to me. The only other > thing I can think of is providing some wrappers (e.g., > ByteString_To_Mesos_Protobuf_2_5_0_ByteString) but since this issue seems > like it will boil down to an individual project and whether or not they know > they have this problem (or one of their users has this problem) they should > be able to start with the un-shaded jar and move to the shaded jar on demand > (and make the protobuf changes then). I've added Patrick Wendell to this > review to see if there was anything they did in particular for Spark to deal > with this case. > > Benjamin Hindman wrote: > FYI, I updated Jenkins so hopefully the review bot will pick this up and > test it for us. > > Tobi Knaup wrote: > Thanks! A wrapper would work too, I'd argue though that a namespace > change is less invasive and easier to revert later.
I agree, let's go with the two jars. Also, I think I'd prefer to keep the examples using the non-shaded protobuf for now since that's what we "prefer" people use/do. We could add a TestFrameworkShaded.java to test the shaded version but feel free to punt for a subsequent review (and/or add a TODO in the makefile where we build the examples suggesting as much). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20402/#review40553 ----------------------------------------------------------- On April 16, 2014, 4:24 p.m., Tobi Knaup wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20402/ > ----------------------------------------------------------- > > (Updated April 16, 2014, 4:24 p.m.) > > > Review request for mesos, Benjamin Hindman and Patrick Wendell. > > > Bugs: MESOS-1212 > https://issues.apache.org/jira/browse/MESOS-1212 > > > Repository: mesos-git > > > Description > ------- > > Use Maven to build the Mesos jars, and include shaded protbuf jar. > No longer builds a custom protobuf jar but relies on Maven. > > NOTE: #20329 (shading) is a breaking change for JVM frameworks which use the > com.google.protobuf namespace. These need to change to > org.apache.mesos.com.google.protobuf. > > Includes the dependent patch from https://reviews.apache.org/r/20329/ which > I'll delete once that's merged. > > > Diffs > ----- > > configure.ac c1de6d7 > src/Makefile.am 560b4c7 > src/examples/java/TestExceptionFramework.java 464b3b0 > src/examples/java/TestFramework.java 65ee2dc > src/examples/java/TestMultipleExecutorsFramework.java 6846959 > src/examples/java/test-exception-framework.in 26617e2 > src/examples/java/test-executor.in 8b27a37 > src/examples/java/test-framework.in bbdc1ed > src/examples/java/test-log.in b7e69e4 > src/examples/java/test-multiple-executors-framework.in eb8edf6 > src/java/mesos.pom.in 8f9b747 > > Diff: https://reviews.apache.org/r/20402/diff/ > > > Testing > ------- > > make check > make maven-install > > > Thanks, > > Tobi Knaup > >
