Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Move some common files out of rdsn directory #1079

Merged
merged 2 commits into from
Jul 26, 2022
Merged

refactor: Move some common files out of rdsn directory #1079

merged 2 commits into from
Jul 26, 2022

Conversation

acelyc111
Copy link
Member

@acelyc111 acelyc111 commented Jul 25, 2022 •
edited
Loading

#1053

  • move rdsn docs to ./docs
  • move common *.thrift to ./idl
  • move thrift compilation scripts to ./scripts

@acelyc111 acelyc111 changed the title refactor: unify dirs refactor: Move some common files out of rdsn directory Jul 25, 2022
@acelyc111 acelyc111 marked this pull request as ready for review July 25, 2022 11:08
@acelyc111 acelyc111 closed this Jul 25, 2022
@acelyc111 acelyc111 reopened this Jul 25, 2022
neverchanje
neverchanje reviewed Jul 26, 2022
View reviewed changes
src/rdsn/src/runtime/security/security.thrift
@@ -15,7 +15,7 @@
// specific language governing permissions and limitations
// under the License.

include "../../dsn.thrift"
include "../../../../../idl/dsn.thrift"
Copy link
Contributor

Choose a reason for hiding this comment

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

dsn.thrift in rdsn/src/ looks fine to me, why did you move it to an outer directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

dsn.thrift contains some common thrift structures which will be used for not only rdsn, but also client libs. Its name 'dsn.thrift' is not accurate, maybe 'base.thrift' would be better, like it named in client lib directories(e.g. https://github.com/apache/incubator-pegasus/blob/master/java-client/idl/base.thrift).

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, we will refactor the code and only leave one unique base.thrift in Pegasus later.

neverchanje
neverchanje approved these changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@neverchanje neverchanje left a comment

Choose a reason for hiding this comment

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

rest lgtm

@neverchanje
Copy link
Contributor 

[ RUN      ] meta_bulk_load_http_test.start_compaction_test
got signal id: 11
Segmentation fault (core dumped)

Seems bulkload has some bugs.

@acelyc111
Copy link
Member Author

start_compaction_test

not related to this pr, we tracked it by issue, #1060

@acelyc111 acelyc111 merged commit b8e4cd6 into apache:master Jul 26, 2022
acelyc111 added a commit to acelyc111/pegasus that referenced this pull request Jul 27, 2022
@acelyc111
refactor: Move some common files out of rdsn directory (apache#1079)
68761a6
acelyc111 added a commit to acelyc111/pegasus that referenced this pull request Jul 27, 2022
@acelyc111
refactor: Move some common files out of rdsn directory (apache#1079)
7284bf7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@foreverneverer foreverneverer foreverneverer approved these changes

@neverchanje neverchanje neverchanje approved these changes

@empiredan empiredan Awaiting requested review from empiredan

@levy5307 levy5307 Awaiting requested review from levy5307

Assignees
No one assigned
Labels
build docs scripts thrift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@acelyc111 @neverchanje @foreverneverer