-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Event Hubs] remove createFromIotHubConnectionString method #5311
Conversation
sdk/eventhub/event-hubs/README.md
Outdated
|
||
```javascript | ||
const client = await EventHubClient.createFromIotHubConnectionString("connectionString"); | ||
const client = new EventHubClient("connectionStringWithEntityPath"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have an example of the connection string format above - maybe just paste it in here? The eye gets drawn to the sample more than the description of the sample.
sdk/eventhub/event-hubs/changelog.md
Outdated
|
||
- Removed the `createFromIotHubConnectionString` method from `EventHubClient`. ([PR #5311](https://github.com/Azure/azure-sdk-for-js/pull/5311)). | ||
Instead, pass an Event Hubs-compatible connection string when instantiating an `EventHubClient` to read properties or events | ||
from an IoT Hub. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be useful here to show what the 'old' vs 'new' code difference is in a little snippet.
This one is pretty simple though.
await client.getProperties(); | ||
await client.getPartitionProperties("partitionId"); | ||
``` | ||
|
||
**Notes:** For scalable and efficient receiving, please take a look at [azure-event-processor-host](https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/eventhub/event-processor-host). The Event Processor host, internally uses the streaming receiver to receive events. | ||
**Notes:** For scalable and efficient receiving, please take a look at [EventProcessor](https://azure.github.io/azure-sdk-for-js/event-hubs/classes/eventprocessor.html). The EventProcessor, internally uses the batched receiver to receive events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious - what makes this more scalable/efficient? I looked at the linked doc and it doesn't mention it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better link would be https://docs.microsoft.com/en-us/azure/event-hubs/event-hubs-event-processor-host once the content there has been updated to be language agnostic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you recommending that I wait to change the link until after the docs there have been updated? Right now those docs are specific to EPH, which we've updated significantly as EventProcessor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend we wait until the docs there have been updated. Maybe log an issue so that we dont forget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logged #5355 to track this work.
sdk/eventhub/event-hubs/README.md
Outdated
@@ -276,15 +276,21 @@ To control the number of events passed to processEvents, use the options argumen | |||
You can use `EventHubClient` to work with IotHub as well. This is useful for receiving telemetry data of IotHub from the linked EventHub. | |||
Most likely the associated connection string will not have send claims. Hence getting HubRuntimeInfo or PartitionRuntimeInfo and receiving events would be the possible operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hence getting HubRuntimeInfo or PartitionRuntimeInfo and receiving events would be the possible operations.
Should we flip this and say that "send operations are not possible"?
sdk/eventhub/event-hubs/README.md
Outdated
@@ -276,15 +276,21 @@ To control the number of events passed to processEvents, use the options argumen | |||
You can use `EventHubClient` to work with IotHub as well. This is useful for receiving telemetry data of IotHub from the linked EventHub. | |||
Most likely the associated connection string will not have send claims. Hence getting HubRuntimeInfo or PartitionRuntimeInfo and receiving events would be the possible operations. | |||
|
|||
- Please notice that we are awaiting on the [createFromIotHubConnectionString](https://azure.github.io/azure-sdk-for-js/event-hubs/classes/eventhubclient.html#createfromiothubconnectionstring) method to get an instance of the EventHubClient. This is different from other static methods on the client. The method talks to the IotHub endpoint to get a redirect error which contains the EventHub endpoint to talk to. It then constructs the right EventHub connection string based on the information in the redirect error and returns an instance of the EventHubClient that you can play with. | |||
- Please notice that the connection string needs to contain the `Endpoint` key-value pair | |||
in addition to the `SharedAccessKeyName` and `SharedAccessKey` key-value pairs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead talk directly about the "Built in endpoints" or "event hub compatible endpoints" instead?
If they have the iot hub specific endpoint, and they read this line, we are telling them that it wont work, but we are not telling them how to fix the issue and move ahead
sdk/eventhub/event-hubs/README.md
Outdated
- Please notice that we are awaiting on the [createFromIotHubConnectionString](https://azure.github.io/azure-sdk-for-js/event-hubs/classes/eventhubclient.html#createfromiothubconnectionstring) method to get an instance of the EventHubClient. This is different from other static methods on the client. The method talks to the IotHub endpoint to get a redirect error which contains the EventHub endpoint to talk to. It then constructs the right EventHub connection string based on the information in the redirect error and returns an instance of the EventHubClient that you can play with. | ||
- Please notice that the connection string needs to contain the `Endpoint` key-value pair | ||
in addition to the `SharedAccessKeyName` and `SharedAccessKey` key-value pairs. | ||
It can optionally include the name of the IoT Hub as the `EntityPath` value, or the name can be passed separately to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value used from the portal has the EntityPath
set in it.
There is no other way that the user can get the event hub compatible connection string.
Is there any value talking about EntityPath
being optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I know how to get the event-hubs compatibile connection string through the portal! When I was getting it through an ARM template, I had to add EntityPath
manually and through testing found it wasn't required if you specify it in the constructor instead.
await client.getProperties(); | ||
await client.getPartitionProperties("partitionId"); | ||
``` | ||
|
||
**Notes:** For scalable and efficient receiving, please take a look at [azure-event-processor-host](https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/eventhub/event-processor-host). The Event Processor host, internally uses the streaming receiver to receive events. | ||
**Notes:** For scalable and efficient receiving, please take a look at [EventProcessor](https://azure.github.io/azure-sdk-for-js/event-hubs/classes/eventprocessor.html). The EventProcessor, internally uses the batched receiver to receive events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better link would be https://docs.microsoft.com/en-us/azure/event-hubs/event-hubs-event-processor-host once the content there has been updated to be language agnostic
@@ -12,7 +12,9 @@ import { EnvVarKeys, getEnvVars } from "./utils/testUtils"; | |||
const env = getEnvVars(); | |||
|
|||
describe("EventHub Client with iothub connection string ", function(): void { | |||
const service = { connectionString: env[EnvVarKeys.IOTHUB_CONNECTION_STRING] }; | |||
const service = { | |||
connectionString: (env[EnvVarKeys.IOTHUB_CONNECTION_STRING] as string) || "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we run the tests in CI?
I wonder if this env value is using the event hub compatible connection string or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, only locally. I need to find out how to actually change it for the CI system :)
sdk/eventhub/event-hubs/README.md
Outdated
const client = await EventHubClient.createFromIotHubConnectionString("connectionString"); | ||
const client = new EventHubClient( | ||
"Endpoint=sb://my-iothub-namespace-[uid].servicebus.windows.net/;SharedAccessKeyName=my-SA-name;SharedAccessKey=my-SA-key;EntityPath=my-iot-hub-name" | ||
); | ||
await client.getProperties(); | ||
await client.getPartitionProperties("partitionId"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"partitionId" as a string wont work here. This sample needs updating...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
27a6989
to
db3cb41
Compare
/azp run js - eventhubs-client - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run js - eventhubs-client - tests |
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #5203
Fixes #5219
Fixes #3473
This change removes the
EventHubClient.createFromIotHubConnectionString
method. Instead, an event-hubs compatible connection string can be used directly when instantiating anEventHubClient
.