Skip to content

[EventHubs] Eventhubs aio changes#8331

Merged
lmazuel merged 19 commits intoAzure:feature/eventhubs_preview5from
YijunXieMS:eventhubs_preview5_aio_review
Nov 4, 2019
Merged

[EventHubs] Eventhubs aio changes#8331
lmazuel merged 19 commits intoAzure:feature/eventhubs_preview5from
YijunXieMS:eventhubs_preview5_aio_review

Conversation

@YijunXieMS
Copy link
Contributor

@YijunXieMS YijunXieMS commented Oct 31, 2019 •
edited
Loading

This PR only has the async code chagne under azure.eventhub.aio.
To avoid too many files to review, I created this PR that only has the core code.

To make it testable, I changed some sync files, mainly change EventHubClient.create_consumer to ._create_consumer and create_producer to _create_producer. You can skip fast for these changes.

@adxsdk6
Copy link

adxsdk6 commented Oct 31, 2019

Can one of the admins verify this patch?

@YijunXieMS YijunXieMS requested a review from lmazuel October 31, 2019 23:18
johanste
johanste reviewed Nov 1, 2019
View reviewed changes
sdk/eventhub/azure-eventhubs/azure/eventhub/aio/_connection_manager_async.py


def get_connection_manager(**kwargs):
connection_mode = kwargs.get("connection_mode", _ConnectionMode.SeparateConnection)
Copy link
Member

@johanste johanste Nov 1, 2019

Choose a reason for hiding this comment

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

The **kwargs passed in here comes from the constructor of the EventHubClient. How is the caller supposed to pass in a valid value here given that the type is internal?

Copy link
Contributor Author

@YijunXieMS YijunXieMS Nov 1, 2019

Choose a reason for hiding this comment

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

We get either users' input, for instance, http_proxy, or the default value of uamqp if it's not in users' input

Copy link
Member

@johanste johanste Nov 1, 2019

Choose a reason for hiding this comment

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

What does the user's code look like if they want to use websockets?

Copy link
Contributor Author

@YijunXieMS YijunXieMS Nov 3, 2019

Choose a reason for hiding this comment

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

EventHubConsumerClient(..., transport_type = TransportType.WebSocket)

Copy link
Member

@lmazuel lmazuel Nov 4, 2019

Choose a reason for hiding this comment

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

Not blocking for preview5, solving

Copy link
Member

@johanste johanste Nov 4, 2019

Choose a reason for hiding this comment

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

Sorry. Meant to ask about connection_mode, not transport.

Copy link
Member

@johanste johanste Nov 4, 2019

Choose a reason for hiding this comment

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

(not blocking the release, but I would still like to get an answer :))

Copy link
Contributor Author

@YijunXieMS YijunXieMS Nov 5, 2019

Choose a reason for hiding this comment

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

connection_mode is not for external users. So our documentation doesn't mention it. We use it.

Copy link
Member

@johanste johanste Nov 5, 2019

Choose a reason for hiding this comment

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

...so we smuggle a **kwarg between internal parts of our code? The signature of get_connection_manager is suspect to say the least. Why not take a single connection_mode optional positional parameter with a default value? As it is now, the code is not very maintainable.

annatisch
annatisch reviewed Nov 1, 2019
View reviewed changes
@yunhaoling yunhaoling changed the title Eventhubs aio changes [EventHubs] Eventhubs aio changes Nov 1, 2019
@lmazuel lmazuel changed the base branch from eventhubs_preview5 to feature/eventhubs_preview5 November 4, 2019 17:18
@lmazuel lmazuel merged commit 8403743 into Azure:feature/eventhubs_preview5 Nov 4, 2019
yunhaoling pushed a commit that referenced this pull request Nov 5, 2019
@lmazuel @yunhaoling
[EventHubs] Preview 5 (#8380)
a6340bd 
* [EventHubs] Eventhubs Sync Changes (#8336)

* Sync EventHub API for review

* 2.7 compatible

* new api tests

* Update sync version implemention and tests

* Making CI happy

* remove async file and add README CHANGELOG

* Update common files in eventprocessor

* Adding checkpointstoreblob

* Update unittest and share_requirement

* Update shared_requirements.txt

* Update setup.py for CI to pass

* Update storage dependency

* Update according to comment

* [EventHubs] Eventhubs aio changes (#8331)

* Eventhubs AIO changes

* Add new files

* Add new files

* Add test code for review

* Changes for code review

* Make it testable

* Update conftest for backward compatibility

* add init to _eventprocessor

* Remove obsolete async test example

* Remove sample partition manager

* update blobstorageaio

* Put close reason and ownership error to common

* Add blobstorage stuffs

* fix pylint

* Fix shared_requirement

* Change blob storage dependency to >=12.0.0

* small doc change

* Doc for eventhub

* More doc improvement

* Fix doc folder detection

* Doc improvement

* [EventHubs] Updating samples, small update in readme (#8383)

* Updating samples, small update in readme

* Small update on blob storage sample

* Update readme to fix analyze check

* remove timeout and retry in sample

* fix pylint and update rst

* remove timeout and retry in sample

* Remove CloseReason from aio init

* Update docstring

* Hide static keys of EventData

* Update from connection string and test.yml (#8392)

* update env name in yml

* update from connection string

* small fix

* Update docstring

* Fix a test file problem after merge async/sync

* Docstring, sample, readme update (#8395)

* update env name in yml

* update from connection string

* small fix

* Update manifest files, samples, code snippet

* Fix in docstring

* update readme

* Small fix

* remove unused import

* sync and async sample balance load

* Changelog, sample, test updates (#8411)

* doc feedback update

* update on changelog and samples

* small fix in samples

* Revert "Fix doc folder detection"

This reverts commit b3c601f.
@YijunXieMS YijunXieMS deleted the eventhubs_preview5_aio_review branch December 16, 2019 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lmazuel lmazuel lmazuel left review comments

@annatisch annatisch annatisch left review comments

@johanste johanste johanste left review comments

@yunhaoling yunhaoling Awaiting requested review from yunhaoling

Assignees

No one assigned

Labels

Event Hubs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@YijunXieMS @adxsdk6 @lmazuel @annatisch @johanste @yunhaoling