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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
