Skip to content

[INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar#7171

Merged
dockerzhang merged 3 commits intoapache:masterfrom
healchow:master
Jan 8, 2023
Merged

[INLONG-5776][Manager] Add tenant param to InlongGroup that uses Pulsar#7171
dockerzhang merged 3 commits intoapache:masterfrom
healchow:master

Conversation

@healchow
Copy link
Member

@healchow healchow commented Jan 5, 2023

Prepare a Pull Request

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

  • This change is a trivial rework/code cleanup without any test coverage.

Documentation

  • Does this pull request introduce a new feature? (no)

@healchow healchow requested a review from gong January 6, 2023 06:39
@gong
Copy link
Contributor

gong commented Jan 6, 2023

@chestnut-c please help to review it

@vernedeng
Copy link
Contributor

vernedeng commented Jan 6, 2023

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".

@dockerzhang dockerzhang marked this pull request as draft January 6, 2023 11:25
@healchow
Copy link
Member Author

healchow commented Jan 7, 2023

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:

/**
* TODO Add default value InlongConstants.DEFAULT_PULSAR_TENANT when you remove the 'tenant'
* from {@link org.apache.inlong.manager.pojo.cluster.pulsar.PulsarClusterRequest}
*/
@ApiModelProperty(value = "Pulsar tenant")
private String tenant;

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.

Copy link
Contributor

@vernedeng vernedeng left a comment

Choose a reason for hiding this comment

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

lgtm

@dockerzhang dockerzhang marked this pull request as ready for review January 8, 2023 14:02
@dockerzhang dockerzhang merged commit 3f36703 into apache:master Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improve][Manager] Add tenant param for Pulsar info

7 participants