You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "satansk (GitHub)" <gi...@apache.org> on 2018/10/09 05:02:11 UTC

[GitHub] [incubator-dubbo] satansk opened pull request #2618: [Dubbo-1983] Support Protobuf Serialization

## What is the purpose of the change

close #1983

## Brief changelog

This Protobuf support is based on [Protostuff](https://github.com/protostuff/protostuff), so we don't need to write proto files by hand.

And this PR is based on @vangoleo 's demo [here](https://github.com/vangoleo/incubator-dubbo/tree/protobuf).

Maybe some benchmarks will be helpful, and I will add them later.

## Verifying this change

XXXXX

Follow this checklist to help us incorporate your contribution quickly and easily:

- [x] Make sure there is a [GITHUB_issue](https://github.com/apache/incubator-dubbo/issues) filed for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
- [x] Format the pull request title like `[Dubbo-XXX] Fix UnknownException when host config not exist #XXX`. Each commit in the pull request should have a meaningful subject line and body.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in [test module](https://github.com/apache/incubator-dubbo/tree/master/dubbo-test).
- [x] Run `mvn clean install -DskipTests` & `mvn clean test-compile failsafe:integration-test` to make sure unit-test and integration-test pass.
- [ ] If this contribution is large, please follow the [Software Donation Guide](https://github.com/apache/incubator-dubbo/wiki/Software-donation-guide).


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] jliu666 commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "jliu666 (GitHub)" <gi...@apache.org>.
@satansk When can you finish your pull request? I would like to use it.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
license

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] Jeff-Lv commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "Jeff-Lv (GitHub)" <gi...@apache.org>.
I found a quote from [stackoverflow ](https://stackoverflow.com/questions/38245281/protobuf-payload-bigger-than-json)

I found the using of protobufIOUtil instead of ProtostuffIoUtil, but still confusing is there any good support  protocol buffer for the Protostuff with popular using? 

`
Protostuff and Protobuf are not the same thing. Protostuff is a wrapper library that can use many different serialization formats. It also supports runtime schema generation, which you appear to be using. That runtime schema requires sending extra metadata along with the message to tell the receiver about the message's schema. I would guess that the large message you're seeing is mostly from this runtime schema data.

With standard Protobuf, the schema is not sent with the message because it is assumed that the sender and recipient already agree on a schema provided by a .proto file compiled into both programs. If you use Protobuf with a standard .proto file, you'll find that the messages it produces are much smaller than JSON.
`

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] satansk commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "satansk (GitHub)" <gi...@apache.org>.
> I find several class in dubbo-serialization module.
> I will start a work on optimize it. Issue:
> #2623
> 
> After that, we can make this pr's test simply.

This is very good, these serialization modules have almost the same unit tests, it's good to simplify it.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
use english

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
I find several class in dubbo-serialization module.
I will start a work on optimize it. Issue:
https://github.com/apache/incubator-dubbo/issues/2623

After that, we can make this pr's test simply.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] satansk commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "satansk (GitHub)" <gi...@apache.org>.
Hi @beiwei30 , this PR still need the following improvements:

* the unit test failed on `java.sql.time` and pojo which has loop reference;
* use cache to improvement performance just as @Jeff-Lv commented;

And sorry for the delay, because I'm preparing a release in my company :)

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] satansk commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "satansk (GitHub)" <gi...@apache.org>.
@carryxyh The pojos you mention are for tests, and it seems necessary :)

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
@satansk failed to response, I decide to merge his change first, then follow up with the pending review comments, what do you think?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
@satansk any update on this?

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] Jeff-Lv commented on pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "Jeff-Lv (GitHub)" <gi...@apache.org>.
The buffer's mainly purpose is for performance with caching. Here is created everytime.

The following is a good sample.
`
 // Re-use (manage) this buffer to avoid allocating on every serialization
    LinkedBuffer buffer = LinkedBuffer.allocate(512);

    // ser
    final byte[] protostuff;
    try
    {
        protostuff = ProtostuffIOUtil.toByteArray(foo, schema, buffer);
    }
    finally
    {
        buffer.clear();
    }
`

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
remove authoer info

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] beiwei30 closed pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "beiwei30 (GitHub)" <gi...@apache.org>.
[ pull request closed by beiwei30 ]

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
Why it necessary?
Pls test it yourself, but submit simplest.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] satansk commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "satansk (GitHub)" <gi...@apache.org>.
Hi @beiwei30 <https://github.com/beiwei30> , this PR still need the following improvements:

the unit test failed on java.sql.time and pojo which has loop reference;
use cache to improvement performance just as @Jeff-Lv <https://github.com/Jeff-Lv> commented;
And sorry for the delay, because I'm preparing a release in my company :) 

I found you are enhancing this PR today, and thanks a lot. Some days later I can be back to the Dubbo community and do some more contributions.

Best regards.

> 在 2018年10月26日，上午10:42，Ian Luo <no...@github.com> 写道：
> 
> @satansk <https://github.com/satansk> failed to response, I decide to merge his change first, then follow up with the pending review comments, what do you think?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/apache/incubator-dubbo/pull/2618#issuecomment-433267786>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD0y76Ft6QzNL-VRphc_HNpTyxdIxJ8Jks5uonaGgaJpZM4XQ9wC>.
> 



[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] satansk commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "satansk (GitHub)" <gi...@apache.org>.
@jliu666 I'm glad you like this feature, but this PR still needs some improvements as @Jeff-Lv commentted, and beside that, I will use the new UT instead of the duplicated pojoes.

I will try to finish it as soon as possible :)

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
I have finish the test of serialization module.
Pls remove the unnecessary POJO class.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
unnecessary comment

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
@satansk 
I will optimize it ASAP. Let us make this pr pending before I finish the ut module. Thx for your contribution.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] carryxyh commented on pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "carryxyh (GitHub)" <gi...@apache.org>.
miss license

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] Jeff-Lv commented on issue #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "Jeff-Lv (GitHub)" <gi...@apache.org>.
I found a quote from [stackoverflow ](https://stackoverflow.com/questions/38245281/protobuf-payload-bigger-than-json)

I found the using of protobufIOUtil instead of ProtostuffIoUtil, but still confusing is there any good support  protocol buffer for the Protostuff with popular using? 

> Protostuff and Protobuf are not the same thing. Protostuff is a wrapper library that can use many different serialization formats. It also supports runtime schema generation, which you appear to be using. That runtime schema requires sending extra metadata along with the message to tell the receiver about the message's schema. I would guess that the large message you're seeing is mostly from this runtime schema data.

> With standard Protobuf, the schema is not sent with the message because it is assumed that the sender and recipient already agree on a schema provided by a .proto file compiled into both programs. If you use Protobuf with a standard .proto file, you'll find that the messages it produces are much smaller than JSON.


[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] Jeff-Lv commented on pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "Jeff-Lv (GitHub)" <gi...@apache.org>.
It is not recommended like this "import io.protostuff.*;" as the checkstyle will report error "Using the '.*' form of import should be avoided"

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org

[GitHub] [incubator-dubbo] Jeff-Lv commented on pull request #2618: [Dubbo-1983] Support Protobuf Serialization

Posted by "Jeff-Lv (GitHub)" <gi...@apache.org>.
advice to refactor here by method extraction because here is a lot of replicated codes.

[ Full content available at: https://github.com/apache/incubator-dubbo/pull/2618 ]
This message was relayed via gitbox.apache.org for notifications@dubbo.apache.org