You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by GitBox <gi...@apache.org> on 2022/12/04 15:56:26 UTC
[GitHub] [incubator-pegasus] acelyc111 opened a new pull request, #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
acelyc111 opened a new pull request, #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284
https://github.com/apache/incubator-pegasus/issues/1283
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] Apache9 commented on a diff in pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#discussion_r1039458922
##########
go-client/generator/admin.csv:
##########
@@ -1,20 +1,20 @@
-RPC_CM_DROP_APP,DropApp,DropAppRequest,DropAppResponse
-RPC_CM_CREATE_APP,CreateApp,CreateAppRequest,CreateAppResponse
-RPC_CM_RECALL_APP,RecallApp,RecallAppRequest,RecallAppResponse
-RPC_CM_LIST_APPS,ListApps,ListAppsRequest,ListAppsResponse
+RPC_CM_DROP_APP,DropApp,ConfigurationDropAppRequest,ConfigurationDropAppResponse
Review Comment:
Let's at least send a NOTICE email to the mailing list first...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#discussion_r1040529249
##########
.github/workflows/lint_and_test_go-client.yml:
##########
@@ -36,27 +36,12 @@ on:
# workflow tasks
jobs:
- lint:
- name: Lint
- runs-on: ubuntu-latest
- steps:
- - uses: actions/checkout@v2
- with:
- fetch-depth: 1
- - name: Set up Go
- uses: actions/setup-go@v2
- with:
- go-version: 1.14
- - name: golangci-lint
- uses: golangci/golangci-lint-action@v3
- with:
- version: v1.29
- working-directory: ./go-client
-
build:
- name: Test
- runs-on: ubuntu-latest
+ name: Test and Lint
+ runs-on: ubuntu:20.04
steps:
+ - name: Install thrift
+ run: sudo apt-get install -y thrift-compiler
Review Comment:
tracked by https://github.com/apache/incubator-pegasus/issues/1283
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#discussion_r1039457292
##########
go-client/generator/admin.csv:
##########
@@ -1,20 +1,20 @@
-RPC_CM_DROP_APP,DropApp,DropAppRequest,DropAppResponse
-RPC_CM_CREATE_APP,CreateApp,CreateAppRequest,CreateAppResponse
-RPC_CM_RECALL_APP,RecallApp,RecallAppRequest,RecallAppResponse
-RPC_CM_LIST_APPS,ListApps,ListAppsRequest,ListAppsResponse
+RPC_CM_DROP_APP,DropApp,ConfigurationDropAppRequest,ConfigurationDropAppResponse
Review Comment:
There are some compatiblity issues, because the structure names changed.
go-client defined it's own idl before, they have the same structure content (the fields type and sequence number) but different names, the go-client side names are more simple and clean, but consider the consistency and maintainability, I think use the unify idl and break some compatiblity would be better, the go-client is not wildly used currently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] neverchanje commented on pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
neverchanje commented on PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#issuecomment-1341139753
I did intentionally name the thrift structs to be different with rDSN. Because the old names in go-client are much shorter, clearer than the rDSN ones, aren't they? 🤣 But certainly, you can rollback to the naming style that is aligned with the core project. I have no opinion with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] Apache9 commented on a diff in pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#discussion_r1039458226
##########
.github/workflows/lint_and_test_go-client.yml:
##########
@@ -36,27 +36,12 @@ on:
# workflow tasks
jobs:
- lint:
- name: Lint
- runs-on: ubuntu-latest
- steps:
- - uses: actions/checkout@v2
- with:
- fetch-depth: 1
- - name: Set up Go
- uses: actions/setup-go@v2
- with:
- go-version: 1.14
- - name: golangci-lint
- uses: golangci/golangci-lint-action@v3
- with:
- version: v1.29
- working-directory: ./go-client
-
build:
- name: Test
- runs-on: ubuntu-latest
+ name: Test and Lint
+ runs-on: ubuntu:20.04
steps:
+ - name: Install thrift
+ run: sudo apt-get install -y thrift-compiler
Review Comment:
`sudo apt-get install -y thrift-compiler='0.13.0-*'`
And we use thrift 0.11.0 in java client?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] Apache9 commented on a diff in pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#discussion_r1039117512
##########
go-client/generator/admin.csv:
##########
@@ -1,20 +1,20 @@
-RPC_CM_DROP_APP,DropApp,DropAppRequest,DropAppResponse
-RPC_CM_CREATE_APP,CreateApp,CreateAppRequest,CreateAppResponse
-RPC_CM_RECALL_APP,RecallApp,RecallAppRequest,RecallAppResponse
-RPC_CM_LIST_APPS,ListApps,ListAppsRequest,ListAppsResponse
+RPC_CM_DROP_APP,DropApp,ConfigurationDropAppRequest,ConfigurationDropAppResponse
Review Comment:
No compatiblity issue?
##########
.github/workflows/lint_and_test_go-client.yml:
##########
@@ -36,27 +36,12 @@ on:
# workflow tasks
jobs:
- lint:
- name: Lint
- runs-on: ubuntu-latest
- steps:
- - uses: actions/checkout@v2
- with:
- fetch-depth: 1
- - name: Set up Go
- uses: actions/setup-go@v2
- with:
- go-version: 1.14
- - name: golangci-lint
- uses: golangci/golangci-lint-action@v3
- with:
- version: v1.29
- working-directory: ./go-client
-
build:
- name: Test
- runs-on: ubuntu-latest
+ name: Test and Lint
+ runs-on: ubuntu:20.04
steps:
+ - name: Install thrift
+ run: sudo apt-get install -y thrift-compiler
Review Comment:
We do not need to specify the version of the thrift compiler?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#issuecomment-1341954730
> I did intentionally name the thrift structs to be different with rDSN. Because the names in go-client are much shorter, clearer than the rDSN ones, aren't they? 🤣 But certainly, you can rollback to the naming style that is aligned with the core project. I have no opinion with that. Or if possible, I suggest that you can rename all ConfigurationXXXResponse to XXXResponse and I think it would make more senses.
I agree, really the go-client side are more tidy. I'm planning to unify all server side and all client sides in following patches at first, and then refactor to a better naming, of course, make sure there is no compatiblity issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] codecov-commenter commented on pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#issuecomment-1338821768
# [Codecov](https://codecov.io/gh/apache/incubator-pegasus/pull/1284?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> :exclamation: No coverage uploaded for pull request base (`master@caf5c1e`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#section-missing-base-commit).
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #1284 +/- ##
=========================================
Coverage ? 53.40%
=========================================
Files ? 27
Lines ? 2522
Branches ? 0
=========================================
Hits ? 1347
Misses ? 1130
Partials ? 45
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#discussion_r1039857851
##########
.github/workflows/lint_and_test_go-client.yml:
##########
@@ -36,27 +36,12 @@ on:
# workflow tasks
jobs:
- lint:
- name: Lint
- runs-on: ubuntu-latest
- steps:
- - uses: actions/checkout@v2
- with:
- fetch-depth: 1
- - name: Set up Go
- uses: actions/setup-go@v2
- with:
- go-version: 1.14
- - name: golangci-lint
- uses: golangci/golangci-lint-action@v3
- with:
- version: v1.29
- working-directory: ./go-client
-
build:
- name: Test
- runs-on: ubuntu-latest
+ name: Test and Lint
+ runs-on: ubuntu:20.04
steps:
+ - name: Install thrift
+ run: sudo apt-get install -y thrift-compiler
Review Comment:
It seems thrift 0.13.0 has been removed from apt for the ubuntu-latest (22.04), I can't even search it.
Java client depends thrift 0.11.0 in pom, and both cpp and java use thrift 0.11.0 to generate code, but cpp link thrift 0.9.3 💊
We can improve and unify the version in next patches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#discussion_r1039444988
##########
.github/workflows/lint_and_test_go-client.yml:
##########
@@ -36,27 +36,12 @@ on:
# workflow tasks
jobs:
- lint:
- name: Lint
- runs-on: ubuntu-latest
- steps:
- - uses: actions/checkout@v2
- with:
- fetch-depth: 1
- - name: Set up Go
- uses: actions/setup-go@v2
- with:
- go-version: 1.14
- - name: golangci-lint
- uses: golangci/golangci-lint-action@v3
- with:
- version: v1.29
- working-directory: ./go-client
-
build:
- name: Test
- runs-on: ubuntu-latest
+ name: Test and Lint
+ runs-on: ubuntu:20.04
steps:
+ - name: Install thrift
+ run: sudo apt-get install -y thrift-compiler
Review Comment:
Yes, so I specify the runner as ubuntu-20.04, on which the thrift-compiler is 0.13.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 merged pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
acelyc111 merged PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1284: refactor(idl): unify the *.thrift files used by cpp and go-client
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on code in PR #1284:
URL: https://github.com/apache/incubator-pegasus/pull/1284#discussion_r1043202676
##########
go-client/generator/admin.csv:
##########
@@ -1,20 +1,20 @@
-RPC_CM_DROP_APP,DropApp,DropAppRequest,DropAppResponse
-RPC_CM_CREATE_APP,CreateApp,CreateAppRequest,CreateAppResponse
-RPC_CM_RECALL_APP,RecallApp,RecallAppRequest,RecallAppResponse
-RPC_CM_LIST_APPS,ListApps,ListAppsRequest,ListAppsResponse
+RPC_CM_DROP_APP,DropApp,ConfigurationDropAppRequest,ConfigurationDropAppResponse
Review Comment:
Will, after this patch merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org