Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

serialization: completely remove protobuf support #161

Merged
merged 9 commits into from
Sep 20, 2018

Conversation

neverchanje
Copy link
Contributor

For historical issue rDSN has an unfinished support for protobuf, however it's been a long time we have no more consideration about it. In this PR I remove all the codes for protobuf and make thrift the only serialization type.
We can re-implement all the stuff when protobuf support becomes a necessity.

Copy link
Contributor

@shengofsun shengofsun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

挺好的。
不过有些没删干净。serialization.h应该还能删一些代码。
移除掉protobuf之后,所有的xxx.types.h也可以删掉的。
先这样吧,等周末我删吧。

@shengofsun
Copy link
Contributor

等下先把cli那个合了吧:-)

@neverchanje
Copy link
Contributor Author

CC @qinzuoyan @acelyc111 帮忙给伟杰先过下

@neverchanje
Copy link
Contributor Author

@shengofsun you can give your suggestion here in detail, so i can work on it tomorrow

@qinzuoyan qinzuoyan mentioned this pull request Sep 13, 2018
28 tasks
@shengofsun
Copy link
Contributor

shengofsun commented Sep 13, 2018

@neverchanje 你可以看下gitlab上,rdsn的refactor分支,你搜一条commit信息里带"serialization“的。
里面主要把各个xxx.types.h下的GENERATED_xxx的宏给删掉了。
另外,在那个commit里,我把几个基本类型的序列化代码都从thirft_helper.h里面拿出去了;这个也可以参考下。

有了这一点,就可以进一步做xxx.types.h的删除了,虽然我当时还没弄。因为我们xxx.types.h只有一个功能:给某个thrift struct添加marshall/unmarshall的函数。这个地方可以改成,include一个<xxx_types.h>,再include一个<serialization.h>就行。

而这么做的好处在于,rdsn自带的code generator也可以干掉了,用官方的thrift就可以了。

我提到的这几点不是特别好改,你可以先不用处理(屎还是一口一口的吃比较好)。先只把pb干掉也挺好。

@neverchanje
Copy link
Contributor Author

neverchanje commented Sep 13, 2018

可以,大致明白你意思。我本来也尽量把每次重构修改的文件数控制到十来个文件左右,这样也好管理,可能一次重构我就分成多个 PR 来做。

@shengofsun shengofsun merged commit 4d47b8d into XiaoMi:master Sep 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants