You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@distributedlog.apache.org by sijie <gi...@git.apache.org> on 2017/05/30 09:12:22 UTC
[GitHub] incubator-distributedlog pull request #132: DL-204: Bump libthrift to latest...
GitHub user sijie opened a pull request:
https://github.com/apache/incubator-distributedlog/pull/132
DL-204: Bump libthrift to latest version for distributedlog-core
Currently finagle heavily depends on an out-of-dated version - libthrift 5.0. Proxy modules (client, server) depend on this version, however the core library doesn't really depend on libthrift.
This change is to change libthrift to 0.9.* in distributedlog-core and shade it to avoid it conflict with the version used by finagle.
This change is based on #131 . The main change is at gitsha [6e58786](https://github.com/apache/incubator-distributedlog/commit/6e587869f87cdce50ae93ba3d52767719d1ab5a6)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sijie/incubator-distributedlog change_thrift_for_core_module
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-distributedlog/pull/132.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #132
----
commit 54c2de047e1656e34ead7fb54070441afd9c140d
Author: Sijie Guo <si...@apache.org>
Date: 2017-05-26T22:05:43Z
Re-organize the distributedlog modules
- move proxy related class from protocol to proxy-protocol, changed client and service to proxy-client and proxy-service
commit 67e76150b103422f16883857a1c1345cf044fb45
Author: Sijie Guo <si...@apache.org>
Date: 2017-05-27T07:04:40Z
Use integration for exception code rather than thrift generated StatusCode
commit 6e587869f87cdce50ae93ba3d52767719d1ab5a6
Author: Sijie Guo <si...@apache.org>
Date: 2017-05-27T07:34:02Z
Use the latest thrift version for distributedlog-core and remove scrooge
commit 03b2abd7bb97d4ca996e51329be3fcc485299416
Author: Sijie Guo <si...@apache.org>
Date: 2017-05-30T08:39:19Z
Fix the testcase related to status code change in exceptions
commit 104ee6f2e496316eee992d0269428924a6028c19
Author: Sijie Guo <si...@apache.org>
Date: 2017-05-30T08:44:18Z
Merge branch 'new_layout' into change_thrift_for_core_module
commit 40cdaadaf25d8ad3df3263e71dd97ad68ed62145
Author: Sijie Guo <si...@apache.org>
Date: 2017-05-30T09:08:07Z
fix module name
----
---
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 wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-distributedlog issue #132: DL-204: Bump libthrift to latest versio...
Posted by leighst <gi...@git.apache.org>.
Github user leighst commented on the issue:
https://github.com/apache/incubator-distributedlog/pull/132
@sijie the usage of thrift in core is pretty limited, seems like
* BKDLConfig etc
* ZKAccessControl etc
Whats the motivation for this change? Just trying to avoid pulling in an old thrift lib when core is used?
---
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 wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-distributedlog pull request #132: DL-204: Bump libthrift to latest...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-distributedlog/pull/132
---
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 wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-distributedlog issue #132: DL-204: Bump libthrift to latest versio...
Posted by leighst <gi...@git.apache.org>.
Github user leighst commented on the issue:
https://github.com/apache/incubator-distributedlog/pull/132
Is this version back compatible with the finagle version?
Do we still use old thrift in proxy-server? Etc.?
Whats the long term plan here? Should we get rid of hard dependency on finagle and modularize the serving component?
---
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 wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-distributedlog issue #132: DL-204: Bump libthrift to latest versio...
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/incubator-distributedlog/pull/132
yes dl-service, dl-client will still depend on twitter maven. because finagle-thrift depends on libthrift-5, which only exists at twitter maven.
---
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 wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-distributedlog issue #132: DL-204: Bump libthrift to latest versio...
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/incubator-distributedlog/pull/132
@leighst a couple of reasons:
- trying to reduce the number of dependencies in distributedlog-core
- the libthrift-5 library only exist in twitter.maven.com. I am in favor of moving to a library that lives in maven central. it is updated with thrift project. no need to make distributedlog-core depend on twitter.maven.com.
---
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 wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-distributedlog issue #132: DL-204: Bump libthrift to latest versio...
Posted by leighst <gi...@git.apache.org>.
Github user leighst commented on the issue:
https://github.com/apache/incubator-distributedlog/pull/132
@sijie dl-service will still depend on twitter maven though right?
---
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 wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-distributedlog issue #132: DL-204: Bump libthrift to latest versio...
Posted by sijie <gi...@git.apache.org>.
Github user sijie commented on the issue:
https://github.com/apache/incubator-distributedlog/pull/132
@leighst : it is format backward compatible, but not binary compatible. I only change the library in distributedlog-core and shade it. The other modules - proxy-protocol, proxy-client and proxy-server that depend on finagle, will still use the libthrift 0.5.
I think the log term plan:
- for proxy-server, it should be able to support multiple protocol bindings: finagle, gRPC, maybe even kafka protocol.
- for client, we might just build different client: finagle one talking to thrift endpoint, gRPC one talking to gRPC endpoint.
---
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 wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---