Skip to content

Conversation

@yunhaoling
Copy link
Contributor

  • Step1: ripping off EventHubClient, EventHubClientAbstract
  • Step2: using uAMQP push model directly

@adxsdk6
Copy link

adxsdk6 commented Nov 7, 2019

Can one of the admins verify this patch?

@bryevdv
Copy link
Contributor

bryevdv commented Nov 18, 2019

At least some of the samples do not currently run:

dev ❯ python recv.py
Traceback (most recent call last):
  File "recv.py", line 66, in <module>
    on_error=on_error)
TypeError: receive() missing 1 required positional argument: 'on_event'

At least in this case the proximate issue seems to be a parameter name change on_events -> on_event but then even if I change that, I see:

Partition: 1 met an exception during receiving: object of type 'EventData' has no len()

I would suggest making sure all the samples run before merging this.

Beyond that, I think this is at the stage where it's probably easier to merge the large PR and follow on with more smaller digestible chunks after.

Copy link
Member

@KieranBrantnerMagee KieranBrantnerMagee left a comment

Choose a reason for hiding this comment

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

Broadly LGTM (for as much detail much as the limited time allowed). The remaining open comments from me are largely nits, so I trust owners to handle reasonably.

Would still sanity check that other reviewers were OK with their resolutions, as I can't speak for them.

@yunhaoling
Copy link
Contributor Author

Hey @bryevdv, the callback now gives you a single event instead of giving you a list of events.
So len(events) would fail.

There are still API changes that needs to be updated after this refactor PR, so we'd prefer update samples after all the changes has been done.
But there are test codes in the branch which are updated and can serve as samples.

@bryevdv
Copy link
Contributor

bryevdv commented Nov 18, 2019

There are still API changes that needs to be updated after this refactor PR, so we'd prefer update samples after all the changes has been done.

It's not what I would do (I think change sets should be as "atomic" as possible and working at every PR merge), but in that case I'd suggest making a separate issue at least.

@annatisch
Copy link
Member

@bryevdv - Agreed and updated. :)

@annatisch
Copy link
Member

/azp run python - eventhubs - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yunhaoling
Copy link
Contributor Author

yunhaoling commented Nov 19, 2019

/azp run python - eventhubs - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

2 similar comments
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants