Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Event Hubs Client] Track Two: Third Preview (AMQP Foundation) #7431

Merged
merged 1 commit into from
Sep 3, 2019
Merged

[Event Hubs Client] Track Two: Third Preview (AMQP Foundation) #7431

merged 1 commit into from
Sep 3, 2019

Conversation

jsquire
Copy link
Member

@jsquire jsquire commented Aug 29, 2019 •
edited
Loading

Summary

The intent of these changes is to continue building out the AMQP foundation, focused on connections, common messages, and management operations.

Goals

  • Handle processing and transformation of data for error scenarios and management operations between the client and service
  • Allow for creation of an AMQP connection, based on the options provided to the EventHubClient
  • Enable management operations by providing the needed infrastructure for them
  • Create an AMQP transport EventHubClient that makes use of the track two infrastructure

Last Upstream Rebase

Tuesday, September 3, 2019 8:47am (EDT)

Related and Follow-Up Issues

@jsquire jsquire added Event Hubs Client This issue points to a problem in the data-plane of the library. labels Aug 29, 2019
@jsquire jsquire added this to the Sprint 158 milestone Aug 29, 2019
@jsquire jsquire self-assigned this Aug 29, 2019
@jsquire
Copy link
Member Author

jsquire commented Aug 29, 2019

//cc: @chrisjbarr

jsquire
jsquire commented Aug 29, 2019
View reviewed changes
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConnectionScope.cs
/// client types within a given scope.
/// </summary>
///
internal class AmqpConnectionScope : IDisposable
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is intended to be the primary manager of connections and associated objects. There are some constructs within that aren't fully utilized yet, since we're only working against the management path. We'll continue to build on this for producer and consumer needs and I'll do a final cleanup pass for any dead items when those are complete.

jsquire
jsquire commented Aug 29, 2019
View reviewed changes
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpConnectionScope.cs

var cbsLink = new AmqpCbsLink(connection);

// TODO (pri2 // squire):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thus far, this is never hit. I want to keep in in place until I finish the producer/consumer work, just in case.

jsquire
jsquire commented Aug 29, 2019
View reviewed changes
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpError.cs
@@ -16,21 +21,6 @@ internal static class AmqpError
/// <summary>Indicates that a timeout occurred on the link.</summary>
public static readonly AmqpSymbol TimeoutError = AmqpConstants.Vendor + ":timeout";

/// <summary>Indicates that a message is no longer available.</summary>
public static readonly AmqpSymbol MessageLockLostError = AmqpConstants.Vendor + ":message-lock-lost";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were all initially ported from Track One code; as I traced them, there were no references outside the mapping. They appear to be Service Bus constructs that were ported over.

@ramya-rao-a , @conniey : Any thoughts based on the error handling from your libraries would be appreciated.

jsquire
jsquire commented Aug 29, 2019
View reviewed changes
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpError.cs
{
// The request timed out.

if (String.Equals(condition, TimeoutError.Value, StringComparison.InvariantCultureIgnoreCase))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in love with this implementation, but I wasn't able to come up with something more elegant. I attempted to order the cases from most likely to least likely to be as efficient as possible, but want to also acknowledge that this is an error path only.

jsquire
jsquire commented Aug 29, 2019
View reviewed changes
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpError.cs

if (String.Equals(condition, AmqpErrorCode.NotFound.Value, StringComparison.InvariantCultureIgnoreCase))
{
if (NotFoundExpression.IsMatch(description)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept this logic to retain the same error behavior as track one. Upon inspection, it seemed useful to keep the distinction.

jsquire
jsquire commented Aug 29, 2019
View reviewed changes
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpEventHubClient.cs
}

ConnectionScope = connectionScope;
ManagementLink = new FaultTolerantAmqpObject<RequestResponseAmqpLink>(timeout => ConnectionScope.OpenManagementLinkAsync(timeout, CancellationToken.None), link => link.SafeClose());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regret the lack of cancellation token here, but could not devise a means to safely and deterministically capture the token that may be in scope when this is called at arbitrary points. Given the lack of cancellation token support in the AMQP library overall, it doesn't cut against the pattern of needing to manually check too badly.

jsquire
jsquire commented Aug 29, 2019
View reviewed changes
sdk/eventhub/Azure.Messaging.EventHubs/src/Amqp/AmqpEventHubClient.cs
///
/// <returns>An Event Hub producer configured in the requested manner.</returns>
///
public override EventHubProducer CreateProducer(EventHubProducerOptions producerOptions,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These NotImplementedExceptions will be covered as part of the work for the AmqpEventHubProducer and AmqpEventHubConsumer which are forthcoming.

jsquire
jsquire commented Aug 29, 2019
View reviewed changes
sdk/eventhub/Azure.Messaging.EventHubs/tests/Amqp/AmqpEventHubClientTests.cs
/// </summary>
///
[Test]
public void GetPartitionPropertiesAsyncCreatesTheRequest()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to limitations in the AMQP library, this was as deep as we could test without providing another layer of abstraction around connections; since this is already a type dedicated to AMQP, that seemed excessive. Full communication will be verified by Live test.

@jsquire jsquire force-pushed the eventhus/tr2p3/amqp-client branch from 19db590 to d9af639 Compare August 30, 2019 13:28
@jsquire jsquire force-pushed the eventhus/tr2p3/amqp-client branch from d9af639 to fae47f4 Compare August 30, 2019 17:52
@jsquire jsquire changed the title [Event Hubs Client] Ttrack Two: Third Preview (AMQP Foundation) [Event Hubs Client] Track Two: Third Preview (AMQP Foundation) Aug 30, 2019
@jsquire jsquire force-pushed the eventhus/tr2p3/amqp-client branch from fae47f4 to 451993b Compare August 30, 2019 19:23
kinelski
kinelski approved these changes Sep 3, 2019
View reviewed changes
Copy link
Member

@kinelski kinelski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left a few nit comments.

@jsquire
[Event Hubs Client] Ttrack Two: Third Preview (AMQP Foundation)
d4fa38c 
  Amqp
    - Implemented the foundation for managing AMQP connections and links
    - Implemented the foundation for identifying and transforming AMQP errors
    - Enabled creation of an AMQP connection using the settings to match client options
    - Enabled creation of a session and link for the management path
    - Enhanced the message converter to handle Event Hub property request/response
    - Enhanced the message converter to handle partition proeprty request/response

  Event Hub Client
    - Created an Amqp transport client, with basic infrastructure
    - Implemented the APIs for requesting metadata from the service

  General
    - Formatting pass over the project
    - Fix naming for a member of hte basic retry policy for consistency
@jsquire jsquire force-pushed the eventhus/tr2p3/amqp-client branch from 451993b to d4fa38c Compare September 3, 2019 13:13
@jsquire jsquire merged commit 4b073b9 into Azure:master Sep 3, 2019
@jsquire jsquire deleted the eventhus/tr2p3/amqp-client branch September 3, 2019 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisjbarr chrisjbarr chrisjbarr left review comments

@kinelski kinelski kinelski approved these changes

@AlexGhiondea AlexGhiondea Awaiting requested review from AlexGhiondea

@conniey conniey Awaiting requested review from conniey

@ramya-rao-a ramya-rao-a Awaiting requested review from ramya-rao-a

@serkantkaraca serkantkaraca Awaiting requested review from serkantkaraca

@ShivangiReja ShivangiReja Awaiting requested review from ShivangiReja

@yunhaoling yunhaoling Awaiting requested review from yunhaoling

@YijunXieMS YijunXieMS Awaiting requested review from YijunXieMS

@chradek chradek Awaiting requested review from chradek

@srnagar srnagar Awaiting requested review from srnagar

Assignees

@jsquire jsquire

Labels
Client This issue points to a problem in the data-plane of the library. Event Hubs
Projects
None yet
Milestone
Sprint 158
Development

Successfully merging this pull request may close these issues.
3 participants
@jsquire @kinelski @chrisjbarr