You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "nandorsoma (via GitHub)" <gi...@apache.org> on 2023/06/26 14:51:45 UTC
[GitHub] [nifi] nandorsoma commented on a diff in pull request #7422: NIFI-11740: Added SnowflakeConfigurationService
nandorsoma commented on code in PR #7422:
URL: https://github.com/apache/nifi/pull/7422#discussion_r1242070493
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/StandardSnowflakeIngestManagerProviderService.java:
##########
@@ -161,17 +188,44 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
.getValue();
final String pipe = context.getProperty(PIPE).evaluateAttributeExpressions().getValue();
fullyQualifiedPipeName = database + "." + schema + "." + pipe;
- final PrivateKeyService privateKeyService = context.getProperty(PRIVATE_KEY_SERVICE)
- .asControllerService(PrivateKeyService.class);
- final PrivateKey privateKey = privateKeyService.getPrivateKey();
-
- final AccountIdentifierFormat accountIdentifierFormat = AccountIdentifierFormat.forName(context.getProperty(ACCOUNT_IDENTIFIER_FORMAT)
- .getValue());
- final AccountIdentifierFormatParameters parameters = getAccountIdentifierFormatParameters(context);
- final String account = accountIdentifierFormat.getAccount(parameters);
- final String host = accountIdentifierFormat.getHostname(parameters);
+
+ final String user;
+ final PrivateKey privateKey;
+ final String account;
+ final String accountUrl;
+
+ final SnowflakeConfigurationService configurationService = context.getProperty(SNOWFLAKE_CONFIGURATION_SERVICE).asControllerService(SnowflakeConfigurationService.class);
+ if (configurationService != null) {
+ final SnowflakeConfiguration configuration = configurationService.getConfiguration();
+ user = configuration.getUsername();
+ privateKey = configuration.getPrivateKey();
+ account = configuration.getAccount();
+ accountUrl = configuration.getAccountUrl();
+ } else {
+ user = context.getProperty(USER_NAME).evaluateAttributeExpressions().getValue();
+
+ final PrivateKeyService privateKeyService = context.getProperty(PRIVATE_KEY_SERVICE).asControllerService(PrivateKeyService.class);
+ privateKey = privateKeyService.getPrivateKey();
+
+ final AccountIdentifierFormat accountIdentifierFormat = AccountIdentifierFormat.forName(context.getProperty(ACCOUNT_IDENTIFIER_FORMAT).getValue());
Review Comment:
I know that this PR didn't touch the forName method, but I wonder `equalsIgnoreCase` is really needed.
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/SnowflakeComputingConnectionPool.java:
##########
@@ -135,6 +136,15 @@ public class SnowflakeComputingConnectionPool extends AbstractDBCPConnectionPool
.description("The password for the Snowflake user.")
.build();
+ public static final PropertyDescriptor SNOWFLAKE_DATABASE = new PropertyDescriptor.Builder()
Review Comment:
It might worth adding a validation that the url doesn't contain db= when this property has a value.
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/util/ConnectionUrlFormat.java:
##########
@@ -22,24 +22,26 @@
import java.util.stream.Stream;
import org.apache.nifi.components.DescribedValue;
+import static org.apache.nifi.snowflake.service.util.SnowflakeConstants.SNOWFLAKE_HOST_SUFFIX;
+
public enum ConnectionUrlFormat implements DescribedValue {
- FULL_URL("full-url", "Full URL", "Provide connection URL in a single property") {
+ JDBC_URL("full-url", "JDBC URL", "Provide connection URL in a single property") {
Review Comment:
Minor, probably JDBC url would be better in the description as well.
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/StandardSnowflakeIngestManagerProviderService.java:
##########
@@ -34,18 +36,38 @@
import org.apache.nifi.expression.ExpressionLanguageScope;
import org.apache.nifi.key.service.api.PrivateKeyService;
import org.apache.nifi.processor.util.StandardValidators;
+import org.apache.nifi.processors.snowflake.SnowflakeConfiguration;
+import org.apache.nifi.processors.snowflake.SnowflakeConfigurationService;
import org.apache.nifi.processors.snowflake.SnowflakeIngestManagerProviderService;
import org.apache.nifi.reporting.InitializationException;
import org.apache.nifi.snowflake.service.util.AccountIdentifierFormat;
import org.apache.nifi.snowflake.service.util.AccountIdentifierFormatParameters;
-import org.apache.nifi.snowflake.service.util.ConnectionUrlFormat;
import org.apache.nifi.processors.snowflake.util.SnowflakeProperties;
+import org.apache.nifi.snowflake.service.util.SnowflakeConstants;
-@Tags({"snowflake", "jdbc", "database", "connection"})
+@Tags({"snowflake", "snowpipe", "ingest"})
Review Comment:
It might worth retaining the "database" and "connection" tags, no?
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/util/ConnectionUrlFormatParameters.java:
##########
@@ -19,19 +19,19 @@
public final class ConnectionUrlFormatParameters extends SnowflakeCommonParameters {
- private final String snowflakeUrl;
+ private final String jdbcUrl;
- public ConnectionUrlFormatParameters(final String snowflakeUrl,
+ public ConnectionUrlFormatParameters(final String jdbcUrl,
Review Comment:
I have the same feeling as at `AccountIdentifierFormat`.
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/util/AccountIdentifierFormatParameters.java:
##########
@@ -19,19 +19,19 @@
public final class AccountIdentifierFormatParameters extends SnowflakeCommonParameters {
- private final String hostUrl;
+ private final String accountUrl;
- public AccountIdentifierFormatParameters(final String hostUrl,
+ public AccountIdentifierFormatParameters(final String accountUrl,
Review Comment:
When I tried to understand the code, the parts related to this class were hard to read. In `AccountIdentifierFormatTest`, I already pointed out that it is a bit weird that 5/6 parameters of this class can be null. I think the root issue is that this class is an unnecessary intermediate step, at least in this form. We need to parse the properties from the context into this POJO, then in the `AccountIdentifierFormat`, we need to declare again which properties we need. The clean way would be to create `*FormatParameter` POJOs for each `AccountIdentifierFormat`. But this could be an overkill. The other option would be removing this class and extracting the required parameters from the context in the `AccountIdentifierFormat` class. Either option is good for me; the latter is probably better because the extra complexity wouldn't bring additional value.
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/StandardSnowflakeIngestManagerProviderService.java:
##########
@@ -196,7 +250,7 @@ public SimpleIngestManager getIngestManager() {
}
private AccountIdentifierFormatParameters getAccountIdentifierFormatParameters(ConfigurationContext context) {
- final String hostUrl = context.getProperty(HOST_URL)
+ final String accountUrl = context.getProperty(ACCOUNT_URL)
Review Comment:
This code block is duplicated in `StandardSnowflakeConfigurationService` and might worth extracting it.
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/util/SnowflakeConstants.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.snowflake.service.util;
+
+public final class SnowflakeConstants {
Review Comment:
I don't see how enum could be used here. Since there are multiple types, the value of the enum should be Object which I think is worse than the current approach. Or am I missing something?
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/StandardSnowflakeIngestManagerProviderService.java:
##########
@@ -161,17 +188,44 @@ public void onEnabled(final ConfigurationContext context) throws InitializationE
.getValue();
final String pipe = context.getProperty(PIPE).evaluateAttributeExpressions().getValue();
fullyQualifiedPipeName = database + "." + schema + "." + pipe;
- final PrivateKeyService privateKeyService = context.getProperty(PRIVATE_KEY_SERVICE)
- .asControllerService(PrivateKeyService.class);
- final PrivateKey privateKey = privateKeyService.getPrivateKey();
-
- final AccountIdentifierFormat accountIdentifierFormat = AccountIdentifierFormat.forName(context.getProperty(ACCOUNT_IDENTIFIER_FORMAT)
- .getValue());
- final AccountIdentifierFormatParameters parameters = getAccountIdentifierFormatParameters(context);
- final String account = accountIdentifierFormat.getAccount(parameters);
- final String host = accountIdentifierFormat.getHostname(parameters);
+
+ final String user;
+ final PrivateKey privateKey;
+ final String account;
+ final String accountUrl;
+
+ final SnowflakeConfigurationService configurationService = context.getProperty(SNOWFLAKE_CONFIGURATION_SERVICE).asControllerService(SnowflakeConfigurationService.class);
+ if (configurationService != null) {
+ final SnowflakeConfiguration configuration = configurationService.getConfiguration();
+ user = configuration.getUsername();
+ privateKey = configuration.getPrivateKey();
+ account = configuration.getAccount();
+ accountUrl = configuration.getAccountUrl();
+ } else {
+ user = context.getProperty(USER_NAME).evaluateAttributeExpressions().getValue();
+
+ final PrivateKeyService privateKeyService = context.getProperty(PRIVATE_KEY_SERVICE).asControllerService(PrivateKeyService.class);
+ privateKey = privateKeyService.getPrivateKey();
+
+ final AccountIdentifierFormat accountIdentifierFormat = AccountIdentifierFormat.forName(context.getProperty(ACCOUNT_IDENTIFIER_FORMAT).getValue());
+ final AccountIdentifierFormatParameters parameters = getAccountIdentifierFormatParameters(context);
+ account = accountIdentifierFormat.getAccount(parameters);
+ accountUrl = accountIdentifierFormat.getAccountUrl(parameters);
+ }
+
+ URL url = null;
+ try {
+ url = new URL(accountUrl);
+ } catch (MalformedURLException e) {
+ // url parse failure => handle accountUrl as hostname only
Review Comment:
+1
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/StandardSnowflakeConfigurationService.java:
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.snowflake.service;
+
+import org.apache.nifi.annotation.documentation.CapabilityDescription;
+import org.apache.nifi.annotation.documentation.Tags;
+import org.apache.nifi.annotation.lifecycle.OnEnabled;
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.controller.AbstractControllerService;
+import org.apache.nifi.controller.ConfigurationContext;
+import org.apache.nifi.expression.ExpressionLanguageScope;
+import org.apache.nifi.key.service.api.PrivateKeyService;
+import org.apache.nifi.processor.util.StandardValidators;
+import org.apache.nifi.processors.snowflake.SnowflakeConfiguration;
+import org.apache.nifi.processors.snowflake.SnowflakeConfigurationService;
+import org.apache.nifi.processors.snowflake.util.SnowflakeProperties;
+import org.apache.nifi.snowflake.service.util.AccountIdentifierFormat;
+import org.apache.nifi.snowflake.service.util.AccountIdentifierFormatParameters;
+import org.apache.nifi.snowflake.service.util.SnowflakeConstants;
+
+import java.security.PrivateKey;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+@Tags({"snowflake"})
+@CapabilityDescription("Provides Snowflake configuration properties")
+public class StandardSnowflakeConfigurationService extends AbstractControllerService implements SnowflakeConfigurationService {
Review Comment:
In `StandardSnowflakeIngestManagerProviderService`, it can be confusing why one property is part of the configuration service and others are not. Therefore I might rename it to `StandardSnowflakeConnectionConfigurationService` to express the scope of the service.
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/test/java/org/apache/nifi/snowflake/service/util/AccountIdentifierFormatTest.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.snowflake.service.util;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class AccountIdentifierFormatTest {
+
+ @Test
+ public void testAccountName() {
+ final AccountIdentifierFormatParameters parameters = new AccountIdentifierFormatParameters(
+ null,
+ "ORG",
+ "ACC",
+ null,
+ null,
+ null);
+
+ final String account = AccountIdentifierFormat.ACCOUNT_NAME.getAccount(parameters);
+ final String accountUrl = AccountIdentifierFormat.ACCOUNT_NAME.getAccountUrl(parameters);
+
+ assertEquals("ORG-ACC", account);
+ assertEquals("ORG-ACC.snowflakecomputing.com", accountUrl);
+ }
+
+ @Test
+ public void testAccountLocator() {
+ final AccountIdentifierFormatParameters parameters = new AccountIdentifierFormatParameters(
+ null,
+ null,
+ null,
+ "LOC",
+ "region",
+ "cloud");
+
+ final String account = AccountIdentifierFormat.ACCOUNT_LOCATOR.getAccount(parameters);
+ final String accountUrl = AccountIdentifierFormat.ACCOUNT_LOCATOR.getAccountUrl(parameters);
+
+ assertEquals("LOC", account);
+ assertEquals("LOC.region.cloud.snowflakecomputing.com", accountUrl);
+ }
+
+ @Test
+ public void testAccountUrl() {
+ final AccountIdentifierFormatParameters parameters = new AccountIdentifierFormatParameters(
Review Comment:
I wonder if it makes sense to have an all args constructor for a class where the standard scenario is that 5 from 6 arguments are null. Builder wouldn't be better?
##########
nifi-nar-bundles/nifi-snowflake-bundle/nifi-snowflake-services/src/main/java/org/apache/nifi/snowflake/service/StandardSnowflakeIngestManagerProviderService.java:
##########
@@ -34,18 +36,38 @@
import org.apache.nifi.expression.ExpressionLanguageScope;
import org.apache.nifi.key.service.api.PrivateKeyService;
import org.apache.nifi.processor.util.StandardValidators;
+import org.apache.nifi.processors.snowflake.SnowflakeConfiguration;
+import org.apache.nifi.processors.snowflake.SnowflakeConfigurationService;
import org.apache.nifi.processors.snowflake.SnowflakeIngestManagerProviderService;
import org.apache.nifi.reporting.InitializationException;
import org.apache.nifi.snowflake.service.util.AccountIdentifierFormat;
import org.apache.nifi.snowflake.service.util.AccountIdentifierFormatParameters;
-import org.apache.nifi.snowflake.service.util.ConnectionUrlFormat;
import org.apache.nifi.processors.snowflake.util.SnowflakeProperties;
+import org.apache.nifi.snowflake.service.util.SnowflakeConstants;
-@Tags({"snowflake", "jdbc", "database", "connection"})
+@Tags({"snowflake", "snowpipe", "ingest"})
@CapabilityDescription("Provides a Snowflake Ingest Manager for Snowflake pipe processors")
public class StandardSnowflakeIngestManagerProviderService extends AbstractControllerService
implements SnowflakeIngestManagerProviderService {
+ public static final PropertyDescriptor USE_SNOWFLAKE_CONFIGURATION_SERVICE = new PropertyDescriptor.Builder()
Review Comment:
I advice renaming it to SNOWFLAKE_CONFIGURATION_METHOD/TYPE with an enumset as allowable values to make this property extendable.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org