You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "turcsanyip (via GitHub)" <gi...@apache.org> on 2023/06/22 08:04:12 UTC
[GitHub] [nifi] turcsanyip opened a new pull request, #7422: NIFI-11740: Added SnowflakeConfigurationService
turcsanyip opened a new pull request, #7422:
URL: https://github.com/apache/nifi/pull/7422
<!-- 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. -->
# Summary
[NIFI-11740](https://issues.apache.org/jira/browse/NIFI-11740)
This PR adds SnowflakeConfigurationService that can be used by multiple Snowflake processors and controller services.
The solution is optimised for NiFi 2.0 where the legacy properties can be removed from StandardSnowflakeIngestManagerProviderService and only "Snowflake Configuration Service" would survive.
For the same reason, some of the properties and code sections are duplicated. I don't think it may worth extracting them in common classes because they will simply be removed from StandardSnowflakeIngestManagerProviderService in 2.0.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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
[GitHub] [nifi] exceptionfactory closed pull request #7422: NIFI-11740: Added SnowflakeConfigurationService
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #7422: NIFI-11740: Added SnowflakeConfigurationService
URL: https://github.com/apache/nifi/pull/7422
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7422: NIFI-11740: Added SnowflakeConfigurationService
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7422:
URL: https://github.com/apache/nifi/pull/7422#discussion_r1238472832
##########
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:
Perhaps this should be logged at the debug level for potential troubleshooting?
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7422: NIFI-11740: Added SnowflakeConfigurationService
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7422:
URL: https://github.com/apache/nifi/pull/7422#discussion_r1238471927
##########
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 recommend using an `enum` opposed to a class with static member variables for constant values.
--
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
[GitHub] [nifi] nandorsoma commented on a diff in pull request #7422: NIFI-11740: Added SnowflakeConfigurationService
Posted by "nandorsoma (via GitHub)" <gi...@apache.org>.
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7422: NIFI-11740: Added SnowflakeConfigurationService
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7422:
URL: https://github.com/apache/nifi/pull/7422#discussion_r1242364992
##########
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:
That's a good point about the multiple types, in that case, `DEFAULT_PORT` would need to return the String `443`.
Either way, I recommend renaming this to something like `SnowflakePropertyValue`. On the other hand, as these values all relate to URI construction, it may be better to refactor usage to something more focused as opposed to creating an unconstrained location for global variables.
--
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