You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/01/30 22:06:44 UTC
[GitHub] [nifi] jfrazee commented on a change in pull request #5303: NIFI-8677 Add custom endpoint suffix for Azure EventHub Processors
jfrazee commented on a change in pull request #5303:
URL: https://github.com/apache/nifi/pull/5303#discussion_r795241858
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -112,7 +112,15 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.required(true)
.build();
- // TODO: Do we need to support custom service endpoints as GetAzureEventHub does? Is it possible?
+ static final PropertyDescriptor SERVICE_BUS_ENDPOINT = new PropertyDescriptor.Builder()
+ .name("Service Bus Endpoint")
+ .description("To support namespaces not in the default windows.net domain.")
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.NONE)
+ .allowableValues("servicebus.windows.net","servicebus.chinacloudapi.cn", "servicebus.cloudapi.de", "servicebus.usgovcloudapi.net")
+ .defaultValue("servicebus.windows.net")
Review comment:
I think it'd be useful to make these `AllowableValue` types and pair these with human readable names like "Azure", "Azure China", "Azure Germany", and "Azure US Government".
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/test/java/org/apache/nifi/processors/azure/eventhub/TestConsumeAzureEventHub.java
##########
@@ -374,7 +375,7 @@ public void testReceiveRecords() throws Exception {
assertEquals(1, provenanceEvents.size());
final ProvenanceEventRecord provenanceEvent1 = provenanceEvents.get(0);
assertEquals(ProvenanceEventType.RECEIVE, provenanceEvent1.getEventType());
- assertEquals("amqps://namespace.servicebus.windows.net/" +
+ assertEquals("amqps://namespace." + serviceBusEndpoint + "/" +
"eventhub-name/ConsumerGroups/consumer-group/Partitions/partition-id", provenanceEvent1.getTransitUri());
Review comment:
Seems like this gets repeated a few times here and below, so it's probably worth creating a pattern for it.
```suggestion
assertEquals(String.format(SOME_PATTERN_CONSTANT, serviceBusEndpoint, provenanceEvent1.getTransitUri()));
```
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -112,7 +112,15 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.required(true)
.build();
- // TODO: Do we need to support custom service endpoints as GetAzureEventHub does? Is it possible?
+ static final PropertyDescriptor SERVICE_BUS_ENDPOINT = new PropertyDescriptor.Builder()
Review comment:
It looks like the same `SERVICE_BUS_ENDPOINT` property is used everywhere here. Is there a reason we can't move it to `AzureEventHubUtils` and pull it in from there for the consume, get, and put? (See `USE_MANAGED_IDENTITY` for example.)
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/PutAzureEventHub.java
##########
@@ -89,6 +89,15 @@
.expressionLanguageSupported(ExpressionLanguageScope.NONE)
.required(true)
.build();
+ static final PropertyDescriptor SERVICE_BUS_ENDPOINT = new PropertyDescriptor.Builder()
Review comment:
Similar comments. Consider moving this to `AzureEventHubUtils`, making the values `AllowableValue`, and changing to `NON_BLANK_VALIDATOR`.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/GetAzureEventHub.java
##########
@@ -99,7 +99,7 @@
.description("To support namespaces in non-standard Host URIs ( not .servicebus.windows.net, ie .servicebus.chinacloudapi.cn) select from the drop down acceptable options ")
.addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
.expressionLanguageSupported(ExpressionLanguageScope.NONE)
- .allowableValues(".servicebus.windows.net",".servicebus.chinacloudapi.cn")
+ .allowableValues(".servicebus.windows.net",".servicebus.chinacloudapi.cn", ".servicebus.cloudapi.de", ".servicebus.usgovcloudapi.net")
Review comment:
Similar comments. Consider moving this to `AzureEventHubUtils`, making the values `AllowableValue`, and changing to `NON_BLANK_VALIDATOR`.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/PutAzureEventHub.java
##########
@@ -241,7 +251,9 @@ protected void waitForAllFutures(
if(flowFileResult.getResult() == REL_SUCCESS) {
final String namespace = context.getProperty(NAMESPACE).getValue();
final String eventHubName = context.getProperty(EVENT_HUB_NAME).getValue();
- session.getProvenanceReporter().send(flowFile, "amqps://" + namespace + ".servicebus.windows.net" + "/" + eventHubName, stopWatch.getElapsed(TimeUnit.MILLISECONDS));
+ final String serviceBusEndpoint = context.getProperty(SERVICE_BUS_ENDPOINT).getValue();
+ final String transitUri = String.format("amqps://%s.%s/%s", namespace, serviceBusEndpoint, eventHubName);
Review comment:
Similar comment about making the pattern a constant.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/GetAzureEventHub.java
##########
@@ -394,7 +394,8 @@ public void onTrigger(final ProcessContext context, final ProcessSession session
final String eventHubName = context.getProperty(EVENT_HUB_NAME).getValue();
final String consumerGroup = context.getProperty(CONSUMER_GROUP).getValue();
final String serviceBusEndPoint = context.getProperty(SERVICE_BUS_ENDPOINT).getValue();
- final String transitUri = "amqps://" + namespace + serviceBusEndPoint + "/" + eventHubName + "/ConsumerGroups/" + consumerGroup + "/Partitions/" + partitionId;
+ final String transitUri = String.format("amqps://%s%s/%s/ConsumerGroups/%s/Partitions/%s",
Review comment:
Maybe pull this pattern out to a constant similar to `ConsumeAzureEventHub`.
##########
File path: nifi-nar-bundles/nifi-azure-bundle/nifi-azure-processors/src/main/java/org/apache/nifi/processors/azure/eventhub/ConsumeAzureEventHub.java
##########
@@ -112,7 +112,15 @@
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.required(true)
.build();
- // TODO: Do we need to support custom service endpoints as GetAzureEventHub does? Is it possible?
+ static final PropertyDescriptor SERVICE_BUS_ENDPOINT = new PropertyDescriptor.Builder()
+ .name("Service Bus Endpoint")
+ .description("To support namespaces not in the default windows.net domain.")
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
Review comment:
It'll be impossible to happen here but in most cases non-blank is what people want since non-empty allows for all whitespace strings.
```suggestion
.addValidator(StandardValidators.NON_BLANK_VALIDATOR)
```
--
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