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

[EventHubs] Partition Manager uses Event Hubs namespace as a key #7637

Merged
merged 13 commits into from
Sep 18, 2019

Conversation

kinelski
Copy link
Member

@kinelski kinelski commented Sep 16, 2019

Current implementations of InMemoryPartitionManager and BlobPartitionManager have three keys (all of them strings) to access content:

  • Event Hub Name
  • Consumer Group
  • Partition Id

These changes include a fourth key, Fully Qualified Namespace (also a string), to avoid ambiguity when a single Partition Manager is being used by Event Processors in different namespace contexts.

The main changes include:

  • Update PartitionOwnership class to include a namespace property.
  • Update Checkpoint class to include a namespace property.
  • Update PartitionContext class to include a namespace property.
  • Update PartitionManager and its derived classes to make use of the new key when managing ownership.
  • Update method calls in the code to support the aforementioned changes.
  • Update PartitionOwnership.IsEquivalentTo extension method to take the new key into consideration when checking equivalence.
  • Include 4 new related tests (2 for InMemoryPartitionManager, 2 for BlobPartitionManager).

@kinelski kinelski added Event Hubs Client This issue points to a problem in the data-plane of the library. labels Sep 16, 2019
@kinelski kinelski added this to the Sprint 159 milestone Sep 16, 2019
@kinelski kinelski requested a review from jsquire September 16, 2019 20:02
@kinelski kinelski self-assigned this Sep 16, 2019
Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

LGTM. A couple of minor thoughts around naming, based on conversations with the service team and internal consistency.

Copy link
Member

@jsquire jsquire left a comment

Choose a reason for hiding this comment

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

Overall still looks good. Shivangi has some great call-outs around a bit of documentation tweaks that we probably want to include.

@@ -91,6 +93,7 @@ await foreach (var response in ContainerClient.GetBlobsAsync(options).ConfigureA
}

ownershipList.Add(new InnerPartitionOwnership(
fullyQualifiedNamespace,
Copy link
Member

@ShivangiReja ShivangiReja Sep 18, 2019

Choose a reason for hiding this comment

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

I think we should add a try catch block around the foreach loop because ContainerClient.GetBlobsAsync(options) can throw an error while fetching the list of blobs.

try {
 await foreach (var response in ContainerClient.GetBlobsAsync(options).ConfigureAwait(false))
            {
                -
                -
            }
            return ownershipList;
  } catch(err) {
    Log($"Error occurred while fetching the list of blobs: '{ err }'");
    throw err;
}

Similarly in updateCheckpoint() method we should throw the errors in catch blocks if any error occurs while updating the checkpoint for the partition. We shouldn't swallow the errors in this case(I believe we swallow these errors in Event Hubs, so it won't stop any process but we should throw from this library)

I know this is not related to your current PR but something to keep in mind

Copy link
Member Author

@kinelski kinelski Sep 18, 2019

Choose a reason for hiding this comment

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

Just took a note. That's something I definitely should revisit when updating the error handling and implementing the Event Processor logging.

@kinelski kinelski merged commit 510a132 into Azure:master Sep 18, 2019
@kinelski kinelski deleted the namespace branch September 18, 2019 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Event Hubs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants