You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@inlong.apache.org by GitBox <gi...@apache.org> on 2023/01/05 15:43:49 UTC
[GitHub] [inlong] healchow opened a new pull request, #7171: [INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar
healchow opened a new pull request, #7171:
URL: https://github.com/apache/inlong/pull/7171
### Prepare a Pull Request
- Fixes #5776
### Motivation
**Add tenant param to InlongGroup that uses Pulsar.**
Currently, the tenant parameter is stored in the Pulsar cluster information, which is not reasonable.
Because a Pulsar cluster supports many tenants, just like its namespaces and topics, **it is a one-to-many relationship with the cluster, not a one-to-one relationship.**
### Modifications
1. Add `tenant` param for `InlongPulsarInfo`.
2. First, get the tenant from `InlongPulsarInfo`, and then get it from the `PulsarCluster`, finally, use the default tenant `public`.
### Verifying this change
- [x] This change is a trivial rework/code cleanup without any test coverage.
### Documentation
- Does this pull request introduce a new feature? (no)
--
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: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] vernedeng commented on pull request #7171: [INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar
Posted by GitBox <gi...@apache.org>.
vernedeng commented on PR #7171:
URL: https://github.com/apache/inlong/pull/7171#issuecomment-1373414529
This is a reasonable change, but it may lead to a configure failure for Sort-Standalone and DataProxy.
--
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: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gong commented on pull request #7171: [INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar
Posted by GitBox <gi...@apache.org>.
gong commented on PR #7171:
URL: https://github.com/apache/inlong/pull/7171#issuecomment-1373334880
@chestnut-c please help to review it
--
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: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7171: [INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #7171:
URL: https://github.com/apache/inlong/pull/7171#discussion_r1063978323
##########
inlong-manager/manager-service/src/main/java/org/apache/inlong/manager/service/listener/consume/apply/ApproveConsumeProcessListener.java:
##########
@@ -130,17 +130,17 @@ private void createPulsarSubscription(InlongConsumeEntity entity) {
ClusterInfo clusterInfo = clusterService.getOne(clusterTag, null, ClusterType.PULSAR);
PulsarClusterInfo pulsarCluster = (PulsarClusterInfo) clusterInfo;
try (PulsarAdmin pulsarAdmin = PulsarUtils.getPulsarAdmin(pulsarCluster)) {
- PulsarTopicInfo topicMessage = new PulsarTopicInfo();
- String tenant = pulsarCluster.getTenant();
+ InlongPulsarDTO pulsarDTO = InlongPulsarDTO.getFromJson(groupEntity.getExtParams());
+ String tenant = pulsarDTO.getTenant();
if (StringUtils.isEmpty(tenant)) {
Review Comment:
Modified.
--
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: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] gosonzhang commented on a diff in pull request #7171: [INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar
Posted by GitBox <gi...@apache.org>.
gosonzhang commented on code in PR #7171:
URL: https://github.com/apache/inlong/pull/7171#discussion_r1063365788
##########
inlong-manager/manager-service/src/main/java/org/apache/inlong/manager/service/listener/consume/apply/ApproveConsumeProcessListener.java:
##########
@@ -130,17 +130,17 @@ private void createPulsarSubscription(InlongConsumeEntity entity) {
ClusterInfo clusterInfo = clusterService.getOne(clusterTag, null, ClusterType.PULSAR);
PulsarClusterInfo pulsarCluster = (PulsarClusterInfo) clusterInfo;
try (PulsarAdmin pulsarAdmin = PulsarUtils.getPulsarAdmin(pulsarCluster)) {
- PulsarTopicInfo topicMessage = new PulsarTopicInfo();
- String tenant = pulsarCluster.getTenant();
+ InlongPulsarDTO pulsarDTO = InlongPulsarDTO.getFromJson(groupEntity.getExtParams());
+ String tenant = pulsarDTO.getTenant();
if (StringUtils.isEmpty(tenant)) {
Review Comment:
isEmpty -- > isBlank
##########
inlong-manager/manager-service/src/main/java/org/apache/inlong/manager/service/consume/ConsumePulsarOperator.java:
##########
@@ -111,12 +112,25 @@ public InlongConsumeInfo getFromEntity(InlongConsumeEntity entity) {
}
String groupId = entity.getInlongGroupId();
InlongGroupInfo groupInfo = groupService.get(groupId);
+ if (!(groupInfo instanceof InlongPulsarInfo)) {
Review Comment:
Why not use mqType to convert directly?
--
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: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on pull request #7171: [INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar
Posted by GitBox <gi...@apache.org>.
healchow commented on PR #7171:
URL: https://github.com/apache/inlong/pull/7171#issuecomment-1374419380
> This is a reasonable change, but it may lead to a configure failure for current SortStandalone and DataProxy. There are two important things to adapt this modification:
>
> 1. A update manuscript from last version to the newest one,
> 2. SortSandalone and DataProxy should change the way to acquire "tenant".
Thanks for your review🎉
As I considered compatibility with DataProxy and SortStandalone, **I did not add the default value for the tenant in the InlongPulsarRequest**, see the code:
https://github.com/apache/inlong/blob/dea5e1fb9d9f9dad572b7a76e9e4e136b6376779/inlong-manager/manager-pojo/src/main/java/org/apache/inlong/manager/pojo/group/pulsar/InlongPulsarRequest.java#L39-L44
That is to say, if you do not set the tenant value when creating or modifying InlongGroup, DataProxy and SortStandalone will read the tenant from the PulsarCluster when they obtain the tenant, because there is no tenant in InlongGroup.
This way should be compatible with historical data. And finally, if you want to upgrade all historical data, you need to modify the request methods of DataProxy / SortStandalone and copy the tenant configuration from PulsarClusters to the related InlongGroups.
--
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: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] dockerzhang merged pull request #7171: [INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar
Posted by GitBox <gi...@apache.org>.
dockerzhang merged PR #7171:
URL: https://github.com/apache/inlong/pull/7171
--
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: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [inlong] healchow commented on a diff in pull request #7171: [INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar
Posted by GitBox <gi...@apache.org>.
healchow commented on code in PR #7171:
URL: https://github.com/apache/inlong/pull/7171#discussion_r1063978404
##########
inlong-manager/manager-service/src/main/java/org/apache/inlong/manager/service/consume/ConsumePulsarOperator.java:
##########
@@ -111,12 +112,25 @@ public InlongConsumeInfo getFromEntity(InlongConsumeEntity entity) {
}
String groupId = entity.getInlongGroupId();
InlongGroupInfo groupInfo = groupService.get(groupId);
+ if (!(groupInfo instanceof InlongPulsarInfo)) {
Review Comment:
It's just a double-check, which seems unnecessary and has been removed.
--
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: commits-unsubscribe@inlong.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org