You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/07/02 22:06:12 UTC
[GitHub] [incubator-pinot] npawar opened a new pull request #5656: Don't override tenantConfig in singleTenantCluster mode, if user explicitly set tenantConfig
npawar opened a new pull request #5656:
URL: https://github.com/apache/incubator-pinot/pull/5656
## Description
We have a flag that can be set when starting the controller called cluster.tenant.isolation. It is by default true. It indicates that the cluster has only 1 tenant called DefaultTenant. If this is set to true, any brokers/servers added to the cluster will have tag DefaultTenant, otherwise they'll have tag server_untagged/broker_untagged. Practically, this flag is true only in testing/quickstart/test setups. All productionized environments would set this to false, and create their own tenants.
In quickstart, after initial run, often times users edit the tenant config (add another tenant or add tagOverrideCofnig). But those settings get overridden because of this flag.
In this pR, Allowing changes to tenant config to come through, even in singleTenantCluster mode
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on pull request #5656: Don't override tenantConfig in singleTenantCluster mode, if user explicitly set tenantConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5656:
URL: https://github.com/apache/incubator-pinot/pull/5656#issuecomment-654376359
> > > You can just remove line 1046-1049 to prevent overriding the tenant config. If there is no tenant specified, it will use `TagNameUtils.DEFAULT_TENANT_NAME` as the default
> >
> >
> > I had to keep those lines, otherwise validation of tenant config would fail (if tenantConfig is null). I removed all the extra conditions in the if, and just kept `if (tableConfig.tenantConfig == null`
>
> Actually i just realized that tenantConfig is Non Nullable in a tableConfig. So removed all those lines
Looks like I'm back to my initial code. The override is necessary, otherwise the table config gets stored in ZK without any tenants.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on pull request #5656: Don't override tenantConfig in singleTenantCluster mode, if user explicitly set tenantConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5656:
URL: https://github.com/apache/incubator-pinot/pull/5656#issuecomment-656426514
https://github.com/apache/incubator-pinot/issues/5615
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on pull request #5656: Don't override tenantConfig in singleTenantCluster mode, if user explicitly set tenantConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5656:
URL: https://github.com/apache/incubator-pinot/pull/5656#issuecomment-654329080
> You can just remove line 1046-1049 to prevent overriding the tenant config. If there is no tenant specified, it will use `TagNameUtils.DEFAULT_TENANT_NAME` as the default
I had to keep those lines, otherwise validation of tenant config would fail (if tenantConfig is null). I removed all the extra conditions in the if, and just kept `if (tableConfig.tenantConfig == null`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on pull request #5656: Don't override tenantConfig in singleTenantCluster mode, if user explicitly set tenantConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5656:
URL: https://github.com/apache/incubator-pinot/pull/5656#issuecomment-654348637
> > You can just remove line 1046-1049 to prevent overriding the tenant config. If there is no tenant specified, it will use `TagNameUtils.DEFAULT_TENANT_NAME` as the default
>
> I had to keep those lines, otherwise validation of tenant config would fail (if tenantConfig is null). I removed all the extra conditions in the if, and just kept `if (tableConfig.tenantConfig == null`
Actually i just realized that tenantConfig is Non Nullable in a tableConfig. So removed all those lines
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5656: Don't override tenantConfig in singleTenantCluster mode, if user explicitly set tenantConfig
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5656:
URL: https://github.com/apache/incubator-pinot/pull/5656#discussion_r450385015
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1044,8 +1044,11 @@ public Schema getTableSchema(String tableName) {
public void addTable(TableConfig tableConfig)
throws IOException {
if (isSingleTenantCluster()) {
Review comment:
Not sure if we should perform this check. Maybe we should always do this null check?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar commented on a change in pull request #5656: Don't override tenantConfig in singleTenantCluster mode, if user explicitly set tenantConfig
Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5656:
URL: https://github.com/apache/incubator-pinot/pull/5656#discussion_r450394866
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java
##########
@@ -1044,8 +1044,11 @@ public Schema getTableSchema(String tableName) {
public void addTable(TableConfig tableConfig)
throws IOException {
if (isSingleTenantCluster()) {
Review comment:
true. Removed isSingleTenantCluster check
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] npawar merged pull request #5656: Don't override tenantConfig in singleTenantCluster mode, if user explicitly set tenantConfig
Posted by GitBox <gi...@apache.org>.
npawar merged pull request #5656:
URL: https://github.com/apache/incubator-pinot/pull/5656
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org