You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/10/10 01:25:01 UTC
[GitHub] [skywalking-banyandb] hanahmily opened a new pull request, #186: Add Exist endpoints to all services
hanahmily opened a new pull request, #186:
URL: https://github.com/apache/skywalking-banyandb/pull/186
Fixes https://github.com/apache/skywalking/issues/8965
Signed-off-by: Gao Hongtao <ha...@gmail.com>
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] wu-sheng commented on a diff in pull request #186: Add Exist endpoints to all services
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #186:
URL: https://github.com/apache/skywalking-banyandb/pull/186#discussion_r990872562
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -233,6 +264,8 @@ service IndexRuleRegistryService {
get:"/v1/index-rule/schema/lists/{group}"
};
};
+ // Exist doesn't expose an HTTP endpoint. Please use HEAD method to touch Get instead
+ rpc Exist(IndexRuleRegistryServiceExistRequest) returns (IndexRuleRegistryServiceExistResponse);
Review Comment:
Why isn't `exist` exposed as a HTTP endpoint?
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -233,6 +264,8 @@ service IndexRuleRegistryService {
get:"/v1/index-rule/schema/lists/{group}"
};
};
+ // Exist doesn't expose an HTTP endpoint. Please use HEAD method to touch Get instead
+ rpc Exist(IndexRuleRegistryServiceExistRequest) returns (IndexRuleRegistryServiceExistResponse);
Review Comment:
Why isn't `exist` exposed as an HTTP endpoint?
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] wu-sheng commented on a diff in pull request #186: Add Exist endpoints to all services
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #186:
URL: https://github.com/apache/skywalking-banyandb/pull/186#discussion_r990896459
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -233,6 +264,8 @@ service IndexRuleRegistryService {
get:"/v1/index-rule/schema/lists/{group}"
};
};
+ // Exist doesn't expose an HTTP endpoint. Please use HEAD method to touch Get instead
+ rpc Exist(IndexRuleRegistryServiceExistRequest) returns (IndexRuleRegistryServiceExistResponse);
Review Comment:
I think there is no point to follow ElasticSearch behavior, banyandb has no logic connection with them
We just need our internally consistent. If this is a service, exposing to get and response as a JSON seems all right.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] wu-sheng merged pull request #186: Add Exist endpoints to all services
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #186:
URL: https://github.com/apache/skywalking-banyandb/pull/186
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #186: Add Exist endpoints to all services
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #186:
URL: https://github.com/apache/skywalking-banyandb/pull/186#discussion_r990896359
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -233,6 +264,8 @@ service IndexRuleRegistryService {
get:"/v1/index-rule/schema/lists/{group}"
};
};
+ // Exist doesn't expose an HTTP endpoint. Please use HEAD method to touch Get instead
+ rpc Exist(IndexRuleRegistryServiceExistRequest) returns (IndexRuleRegistryServiceExistResponse);
Review Comment:
These changes goal is to provide a fancy way to interact with the Java client to avoid throwing a GRPCRuntimeExpection.
If a client leverages HTTP Restful-APIs, the `Get` services are better options for them to issue `HEAD` requests.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] codecov-commenter commented on pull request #186: Add Exist endpoints to all services
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #186:
URL: https://github.com/apache/skywalking-banyandb/pull/186#issuecomment-1272690438
# [Codecov](https://codecov.io/gh/apache/skywalking-banyandb/pull/186?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#186](https://codecov.io/gh/apache/skywalking-banyandb/pull/186?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (11a03d9) into [main](https://codecov.io/gh/apache/skywalking-banyandb/commit/ebdbb62c066223a9e9b106da0c60ea4e50f559b2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ebdbb62) will **decrease** coverage by `0.34%`.
> The diff coverage is `16.12%`.
```diff
@@ Coverage Diff @@
## main #186 +/- ##
==========================================
- Coverage 42.51% 42.17% -0.35%
==========================================
Files 74 74
Lines 7030 7123 +93
==========================================
+ Hits 2989 3004 +15
- Misses 3769 3844 +75
- Partials 272 275 +3
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking-banyandb/pull/186?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [banyand/liaison/grpc/registry.go](https://codecov.io/gh/apache/skywalking-banyandb/pull/186/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YmFueWFuZC9saWFpc29uL2dycGMvcmVnaXN0cnkuZ28=) | `20.13% <16.12%> (-1.82%)` | :arrow_down: |
: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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-banyandb] hanahmily commented on a diff in pull request #186: Add Exist endpoints to all services
Posted by GitBox <gi...@apache.org>.
hanahmily commented on code in PR #186:
URL: https://github.com/apache/skywalking-banyandb/pull/186#discussion_r990894968
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -233,6 +264,8 @@ service IndexRuleRegistryService {
get:"/v1/index-rule/schema/lists/{group}"
};
};
+ // Exist doesn't expose an HTTP endpoint. Please use HEAD method to touch Get instead
+ rpc Exist(IndexRuleRegistryServiceExistRequest) returns (IndexRuleRegistryServiceExistResponse);
Review Comment:
Elasticsearch's `Exist` function uses HTTP's `HEAD` method to query the server index path. If the code is 404, the return value is false. 200 returns true.
Our new `Exist` services always return 200. If a client wants to use the `HEAD` method, it could touch the current `Get` services. They will work as Elasticsearch's behavior.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org