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.

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

mgmt add azure-resourcemanager-eventhubs #14469

Merged
merged 19 commits into from
Aug 31, 2020
Merged

mgmt add azure-resourcemanager-eventhubs #14469

merged 19 commits into from
Aug 31, 2020

Conversation

xseeseesee
Copy link
Contributor

No description provided.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Aug 27, 2020

@xccc-msft Do you have a few files that need deep review? (again, too many files, and changes across multiple commits so it also hard to review by commit).


private Mono<Indexable> createContainerIfNotExistsAsync(final StorageAccount storageAccount,
final String containerName) {
return storageManager.blobContainers()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weidongxu-microsoft This is the main changes after migration. I apply azure-resourcemanager-storage for the scenario to create a container if not exists instead of using azure-storage/azure-storage-blob. The rest part keeps as it is.

Comment on lines 225 to 245
@Override
public EventHubImpl withNewSendRule(final String ruleName) {
addPostRunDependent(context -> manager().eventHubAuthorizationRules()
.define(ruleName)
.withExistingEventHub(ancestor().resourceGroupName(), ancestor().ancestor1Name(), name())
.withSendAccess()
.createAsync()
.last());
return this;
}

@Override
public EventHubImpl withNewListenRule(final String ruleName) {
addPostRunDependent(context -> manager().eventHubAuthorizationRules()
.define(ruleName)
.withExistingEventHub(ancestor().resourceGroupName(), ancestor().ancestor1Name(), name())
.withListenAccess()
.createAsync()
.last());
return this;
}
Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Aug 27, 2020

Choose a reason for hiding this comment

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

Please try test case which do both (NewSend and NewListen) in one Create/Update. I remember service not able to handle concurrent requests under eventhub, so these had to be done in sequence.
Azure/azure-libraries-for-net#891

Choose a reason for hiding this comment

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

Do they have without method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add new method withNewSendAndListenRule(ruleName). There is withoutAuthorizationRule(ruleName) to remove the authorization rule no matter what access it has.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Aug 27, 2020

Choose a reason for hiding this comment

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

Eh, I didn't get it. What I mean is these REST requests under "eventhub" might need to be called sequentially, it might not be limited to this 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discuss offline, we decide to concat the post run tasks in eventhubs.

@xseeseesee xseeseesee merged commit 8880b04 into Azure:master Aug 31, 2020
@xseeseesee xseeseesee deleted the add-mgmt-eventhubs branch August 31, 2020 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants