muga commented 12 years ago

Hi Mikołaj,

I saw your code on mikkoz/msgpack-java.git. It is very nice!

The following is my idea for consolidating your contribution and the existing msgpack-java. The idea is based on subproject-nization (loose coupling). For example,

msgpack-java/
    msgpack-core/
      # a core library of msgpack-java
      # includes org.msgpack.{packer,template,type,unpacker} packages
    msgpack-template-generator/
      # includes org.msgpack.template.builder packages
    msgpack-beans/
      # includes Beans library of the existing msgpack-java
    msgpack-default/
      # all-in-one project
    msgpack-android/
      # includes custom beans (your code is here)
    ...

In my idea, if you execute the following commands, msgpack-java for Android may be created by maven. msgpack-java for Android uses the artifact of the msgpack-android subproject.

$ cd msgpack-java/msgpack-android
$ mvn package

On the other hand, if you execute the following, msgpack-java uses msgpack-beans artifact and is created by maven.

$ cd msgpack-java/msgpack-default
$ mvn package

What do you think about the above idea?

Thanks, Muga Nishizawa

On Mon, Jun 25, 2012 at 3:00 AM, MK spamnij_mnie@interia.pl wrote:

Hi,

done! Here's the code: https://github.com/mikkoz/msgpack-java https://github.com/mikkoz/msgpack-rpc/tree/master/java

Notes: -I haven't managed to test the server on Android with this code yet (it corresponds to the earlier version 'though, and I have tested the client, and surefire is happy when ran). -Comparatively a lot of time now went into determining license compliance with Apache Harmony. I believe everything's OK, but feel free to check that. -I've implemented the solution to the discussed RPC problem as a Factory class. It may be borderline enterprisey, but I think it's the most robust variant (the class is SocketChannelFactoryFactory). -I wasn't sure about the copyright declaration in SocketChannelFactoryFactory, please let me know whether it's OK.

Comments and questions are certainly welcome!

Cheers, Mikołaj

mikolak-net commented 12 years ago

Hi!

Again, I'm glad you like it :).

Regarding the proposed structure - I'm all for modularization of course. However, I'm afraid I haven't encountered the sort of approach you propose here. Usually what I've seen in configurations like this is one of the two:

Either split the platform-specific code into separate projects, and have two effective dependencies, i.e. something like:

msgpack-java/
    msgpack-core/
      # a core library of msgpack-java
      # includes org.msgpack.{packer,template,type,unpacker} packages
    msgpack-template-generator/
      # includes org.msgpack.template.builder packages

    msgpack-default/
      # all-in-one project
msgpack-beans/
   # includes Beans library of the existing msgpack-java
msgpack-android/
   # includes custom beans (your code is here)
    ...

or, create two profiles activated by a property, one adding an "android" classifier to the build artifact, when applicable, something like this (in the POM of msgpack-java):

    <profiles>
        <profile>
            <id>general</id>
            <activation>
                <property>
                    <!-- active when android.build property is NOT set -->
                    <name>!android.build</name>
                </property>
                <!-- rest of the configuration here -->
            </activation>
        </profile>
        <profile>
            <id>android</id>
            <activation>
                <property>
                    <!-- active when android.build property is set -->
                    <name>android.build</name>
                </property>
            </activation>
            <build>
                <plugins>
                    <plugin>
                        <groupId>org.apache.maven.plugins</groupId>
                        <artifactId>maven-jar-plugin</artifactId>
                        <configuration>
                            <classifier>android</classifier>
                            <!-- and so on, and so forth -->
                        </configuration>
                    </plugin>
                </plugins>
            </build>
        </profile>
    </profiles>

The problems I see with the original approach:

I'm afraid there would have to be some configuration duplication between msgpack-default and msgpack-android.
The way I understand it, Maven interprets subfolders with a POM as modules. All modules of a project (msgpack-java, in this case) are built whenever it is built - but the reverse does not hold. So, you would have to trigger the build and install of msgpack-core etc. manually when building either msgpack-default or msgpack-android.

But I may be wrong, feel free to correct me in this case :).

Cheers, Mikołaj

PS. By the way, how should I properly address you on a "first-name" basis?

mikolak-net commented 12 years ago

Hi, I'm going to have some more time for development during the next two weeks, so I'd appreciate a reply until then :).

muga commented 12 years ago

Hi Mikołaj,

I think that your idea is better than mine:-) Platform-specific features should be switched with properties.

And does your msgpack-android code enable to work on any platform? If so, we can replace the beans codes of the current msgpack-java with your custom beans codes.

Thanks, Muga Nishizawa

p.s. My first name is 'Muga'. Please call me 'Muga':-)

mikolak-net commented 12 years ago

Hi Muga :),

thanks for the reply. Regarding platform interops, yes, it should work on any Java implementation - I've tested it on Linux x64 and Android arm. Unit tests pass fine, as well as simple runtime tests (the robot setup I wrote about on the discussion group also had "my", i.e. Project Harmony code on the server side).

I do have one concern 'though, which is speed. The beans implementation contained within the Android fork is simply a re-implementation of the standard beans package. However, I'm not 100% sure that it works as fast as the "standard" implementation. There shouldn't be any visible impact, but it would be good to run some speed tests. if you have any. What do you think?

Cheers, Mikołaj

Dr1Ku commented 12 years ago

Hi Muga, Hi Mikołaj,

I tried to use the jar built from Mikołaj's branch which fixes MessagePack's Android issues after seeing the thread on StackOverflow. While I got it to compile once or twice, when building my project again, Eclipse gets in a really nasty spin, hogging 50% CPU and being generally unresponsive. I got a lot of errors related to the Java heap space, the Workspace being out of memory and such. Other than that, I did manage to see a warning which might pinpoint the issue:

Dx warning: Ignoring InnerClasses attribute for an anonymous inner class (javassist.ClassPool$1) that doesn't come with an associated EnclosingMethod attribute. This class was probably produced by a compiler that did not target the modern .class file format. The recommended solution is to recompile the class from source, using an up-to-date compiler and without specifying any "-target" type options. The consequence of ignoring this warning is that reflective operations on this class will incorrectly indicate that it is not an inner class.

This seems to be coming from javassist, which is included in the Project. Before using Mikołaj's branch I tried with the normal flavor of MessagePack for Java and got the Beans-related errors mentioned on the StackOverflow thread.

I would try to use those annotations you've mentioned, e.g. @MessagePackBeans but I don't know where to place them in my code.

What I was trying to do in my code was unpack a MessagePack-coded String coming from my Ruby backend. I don't think this issue has anything to do with the fact that I a sending an async HTTP GET to my REST backend, yet I thought I would mention it as well.

Any input on this, please? I can supply any other details if required.

Thanks in advance! Cosmin

mikolak-net commented 12 years ago

Cosmin: the message you got is a "feature" ;). At least for now. It's generated by the dx tool, and it means exactly what is says. It shouldn't affect the stability of the build and it certainly shouldn't crash your IDE. Sounds like there's a problem with your Eclipse installation unrelated to msgpack. I'd advise to just mvn package and/or install msgpack-java from the commandline and use it as a dependency (either by linking the JAR or through Maven, depending how you've set up your Android apps), instead of importing it as a project. There's no point in importing it anyway other than to work on the code of msgpack itself.

Regarding @MessagePackBeans, you annotate your data class with it. It means that msgpack recognizes getters and setters as your "fields" in the class. So if you e.g. have getA() in your class, you should have a field "a" in your msgpack object.

Also, if you have any further questions, I'd like to ask you to direct them somewhere else then here (new issue at my branch maybe?), since we're using this issue to discuss the general structure of the project. Speaking of which...

Muga: I'm still waiting for your reply :).

kiyoto commented 12 years ago

Hi all,

I am jumping right into the thread. I just realized I also need Muga to move the needle for this issue too =) My use case is using td-logger-java for Android, and td-logger-java (the Java logger for Treasure Data) in turn depends on msgpack-java.

So yea, I am joining Mikkoz to nudge Muga to work on this =D

mikolak-net commented 11 years ago

I've just attempted to nudge him more directly, let's hope that works :).

muga commented 11 years ago

Hi Mikołaj,

I'm extremely sorry for the late reply. but I returned and checked this thread.

I do have one concern 'though, which is speed. The beans implementation contained within the Android fork is simply a re-implementation of the standard beans package. However, I'm not 100% sure that it works as fast as the "standard" implementation. There shouldn't be any visible impact, but it would be good to run some speed tests. if you have any. What do you think?

hm.. performance of data serialization/deserialization is very important concern as MessagePack project. ok! i try comparing performances of your and standard implementation:-)

please wait.

muga commented 11 years ago

I checked performance of your ReflectionBeansTemplateBuilder and original ReflectionBeansTemplateBuilder. I didn't check performance of JavassistBeansTemplateBuilder because current version of msgpack-java doesn't use it. My program is following: https://gist.github.com/4087169

Two template builder are mostly same performance:-) The details of result of the performance measurement is follwing:

use of ReflectionBeansTemplateBuilder

elapsed time of template creation: 149 elapsed time of serialization: 188 elapsed time of deserialization: 213
use of CustomReflectionBeansTemplateBuilder

elapsed time of template creation: 143 elapsed time of serialization: 193 elapsed time of deserialization: 209

We don't have performance problem. Let's merge your msgpack-android branch into origin branch.

muga commented 11 years ago

I'm wondering about how to register two artifacts (original msgpack-java and your msgpack-android) on Sonatype repository. Do you have good idea?

mikolak-net commented 11 years ago

Hi Muga,

thanks for the reply! It's great to hear that performance is not an issue in this case. But I don't understand the last question in this context - if we're merging the android branch into the main one, we don't need two separate artifacts, at least for the core project, right?

P.S. Sorry for not replying immediately, but I'm recovering from a slight hand injury at the moment. Nothing too serious, I just had to have someone type this message for me.

muga commented 11 years ago

Mikołaj,

if we're merging the android branch into the main one, we don't need two separate artifacts, at least for the core project, right?

Yes, that's a good point. Sorry about misspeaking.

I want to make sure we are all on the same page. Because we don't see any performance problem, we will merge your branch and we will have one repository, one build for all platforms, both Android and non-Android.

If you agree with the above thing, could you send a pull request for merging your msgpack-android branch?

Thanks, Muga

mikolak-net commented 11 years ago

Hi Muga,

sorry for the delay. I'm better now and can type (almost ;p) normally again. I've created the pull request, as linked above.

Now, the question is - what do we do with the Java implementation for msgpack-rpc?

muga commented 11 years ago

Mikołaj,

Thank you:-) I try to merge that into master branch this weekend.

the question is - what do we do with the Java implementation for msgpack-rpc?

Good question, Mikołaj. Let us discuss that on other ticket after releasing next version of msgpack-java. Could you open new ticket for that?

Thanks, Muga

mikolak-net commented 11 years ago

Hi Muga,

great to hear it, thanks :). Let me know if you encounter and problems or inclarities. Regarding msgpack-rpc - should I create the issue within this project (java), or within rpc?

muga commented 11 years ago

Mikołaj,

Please open the issue within on this project:-)

mikolak-net commented 11 years ago

Done :).

22

muga commented 11 years ago

gj, mikołaj:-)

muga commented 11 years ago

Mikołaj,

I renamed your package name ('custom.beans' is a little bit strange) and merged your request #21:-) https://github.com/msgpack/msgpack-java/commits/master

Please check it.

muga commented 11 years ago

next, i will review pull request #22.

muga commented 11 years ago

Hi Mikołaj,

I merged your pull request. Thanks a lot for your help. Actually, I have to apologize you about releasing the new version (with your changes) without your consent. I hope you aren't upset.

Sincerely, Muga

oza commented 11 years ago

This ticket seems to have been closed. I close it now. Please reopen it if there are problems.

mikolak-net commented 11 years ago

Yes, also I don't know why I didn't comment about it - sorry, Muga. Build looks fine as far as I remember.

msgpack / msgpack-java

MessagePack For Android #15

22