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

[Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping #18331

Merged
merged 7 commits into from
Nov 4, 2021

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Oct 23, 2021

By using Jenkins lookup3 algorithm implemented by the jenkins-hash-lookup3 package.

By implementing the Jenkins lookup3 algorithm used by the EH sevice.

TODO: add tests

common/config/rush/pnpm-lock.yaml Outdated Show resolved Hide resolved
sdk/eventhub/event-hubs/src/impl/partitionAssigner.ts Outdated Show resolved Hide resolved
@deyaaeldeen deyaaeldeen requested a review from jsquire November 3, 2021 16:47
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.

Looks to match the service version exactly; it would still be interesting to run a set through the .NET and JS implementations and confirm that we're seeing the same outcomt.

@deyaaeldeen
Copy link
Member Author

it would still be interesting to run a set through the .NET and JS implementations and confirm that we're seeing the same outcomt.

@jsquire I did that and coded those as unit tests in https://github.com/Azure/azure-sdk-for-js/blob/f899ae48ea274cca6f02462ce4efeccc7a77d93b/sdk/eventhub/event-hubs/test/internal/impl/mapPartitionKeyToId.spec.ts.

@jsquire
Copy link
Member

jsquire commented Nov 3, 2021

it would still be interesting to run a set through the .NET and JS implementations and confirm that we're seeing the same outcomt.

@jsquire I did that and coded those as unit tests in https://github.com/Azure/azure-sdk-for-js/blob/f899ae48ea274cca6f02462ce4efeccc7a77d93b/sdk/eventhub/event-hubs/test/internal/impl/mapPartitionKeyToId.spec.ts.

Nice. I mistook that for testing that the implementation produced stable values. You may want to add a note calling out the approach used for clarity.

@deyaaeldeen deyaaeldeen marked this pull request as ready for review November 3, 2021 21:49
@deyaaeldeen deyaaeldeen merged commit 736bdff into feature/eh-buffered-producer Nov 4, 2021
@deyaaeldeen deyaaeldeen deleted the eh-input-partition-ids branch November 4, 2021 00:28

/* eslint-disable no-fallthrough */

import os from "os";
Copy link
Member

Choose a reason for hiding this comment

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

os might not work in browser?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah many things here are node specific such as os and Buffer but the tests passed so I assumed there is polyfilling going on. I will revisit the browser support.

Copy link
Member

Choose a reason for hiding this comment

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

rollup shims perhaps

deyaaeldeen added a commit that referenced this pull request Nov 13, 2021
* [event-hubs] EventHubsBufferedProducerClient skeleton (#17761)

* temp transfer

* [event-hubs] initial EventHubBufferedProducerClient skeleton

* Change default value of BufferedCloseOptions.flush to true instead of false

* Reorder doc comment content for EventHubBufferedProducerClient

* remove eslint exception for @azure/azure-sdk/ts-naming-options in EventHubBufferedProducerClientOptions

* add enqueueEvent (singular) method

* update API view

* [WIP][event-hubs] EventHubBufferedProducerClient implementation (#18106)

* transfer

* [mock-hub] fix issue where calling stop() would not remove existing idle connection timeout watchers

* [event-hubs] reduce repetition in hubruntime.spec.ts tests

* Add support for flush,eventSuccess and eventError handlers, and backpressure to EventHubBufferedProducerClient

* add documentation and update API to align with .NET

* [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping (#18331)

* [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping

* fix the implementation

* factor out the hashing logic

* remove unused import

* the test pass but the implementation needs to be simplified

* simplified implementation

* address feedback

* address feedback

* fix format

* address feedback

Co-authored-by: chradek <[email protected]>
cochi2 added a commit that referenced this pull request Nov 15, 2021
* Removing return response for Pause/Resume/Stop recording

* Removing validations

* Fixes after merge

* Added downloadToFile operation

* Added downloadToFile test

* Updating API MD file

* Fixing format auto

* Renaming files

* [KeyVault] - Move MHSM to resource group location (#18664)

Moving the Managed HSM to the same location as our resource group now that the
limits have been expanded.

* Add default cloud configuration values to source (#18653)

Co-authored-by: Ben Broderick Phillips <[email protected]>

* simplify the commit history so that the patch can apply properly (#18665)

Co-authored-by: scbedd <[email protected]>

* Updating name for header

* [Event Hubs] Merge feature branch for buffered producer (#18590)

* [event-hubs] EventHubsBufferedProducerClient skeleton (#17761)

* temp transfer

* [event-hubs] initial EventHubBufferedProducerClient skeleton

* Change default value of BufferedCloseOptions.flush to true instead of false

* Reorder doc comment content for EventHubBufferedProducerClient

* remove eslint exception for @azure/azure-sdk/ts-naming-options in EventHubBufferedProducerClientOptions

* add enqueueEvent (singular) method

* update API view

* [WIP][event-hubs] EventHubBufferedProducerClient implementation (#18106)

* transfer

* [mock-hub] fix issue where calling stop() would not remove existing idle connection timeout watchers

* [event-hubs] reduce repetition in hubruntime.spec.ts tests

* Add support for flush,eventSuccess and eventError handlers, and backpressure to EventHubBufferedProducerClient

* add documentation and update API to align with .NET

* [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping (#18331)

* [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping

* fix the implementation

* factor out the hashing logic

* remove unused import

* the test pass but the implementation needs to be simplified

* simplified implementation

* address feedback

* address feedback

* fix format

* address feedback

Co-authored-by: chradek <[email protected]>

* [Event Hubs] Prepare release (#18672)

* [Event Hubs] Prepare release

* remove empty sections

* Node doesn't support some samples for smoke test (#18496)

* Node doesn't support some samples for smoke test

* [search-documents] reprocessed samples with exp. generator

Co-authored-by: Will Temple <[email protected]>

* Add rlc quickstart guideline (#18503)

* add llc quickstart guideline

* update format

* add documentation about ci.yml

* update to resolve some comments

* update to resolve some comments

* update to resolve comments

* updates term

* update format

* update format

* Running Rush update

* Sync eng/common directory with azure-sdk-tools for PR 2265 (#18683)

* Improve devops logging for link checker

* Update eng/common/scripts/Verify-Links.ps1

Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>

* Build and test predefined set of packages only in identity pipelines (#18686)

* Build and test predefined set of packages only in identity pipelines

* Fixing playback testing

* Run rushx format

Co-authored-by: Maor Leger <[email protected]>
Co-authored-by: Azure SDK Bot <[email protected]>
Co-authored-by: Ben Broderick Phillips <[email protected]>
Co-authored-by: scbedd <[email protected]>
Co-authored-by: Deyaaeldeen Almahallawi <[email protected]>
Co-authored-by: chradek <[email protected]>
Co-authored-by: Sarangan Rajamanickam <[email protected]>
Co-authored-by: Will Temple <[email protected]>
Co-authored-by: Qiaoqiao Zhang <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: praveenkuttappan <[email protected]>
zihzhan-msft pushed a commit that referenced this pull request Nov 16, 2021
* [KeyVault] - Move MHSM to resource group location (#18664)

Moving the Managed HSM to the same location as our resource group now that the
limits have been expanded.

* Add default cloud configuration values to source (#18653)

Co-authored-by: Ben Broderick Phillips <[email protected]>

* simplify the commit history so that the patch can apply properly (#18665)

Co-authored-by: scbedd <[email protected]>

* [Event Hubs] Merge feature branch for buffered producer (#18590)

* [event-hubs] EventHubsBufferedProducerClient skeleton (#17761)

* temp transfer

* [event-hubs] initial EventHubBufferedProducerClient skeleton

* Change default value of BufferedCloseOptions.flush to true instead of false

* Reorder doc comment content for EventHubBufferedProducerClient

* remove eslint exception for @azure/azure-sdk/ts-naming-options in EventHubBufferedProducerClientOptions

* add enqueueEvent (singular) method

* update API view

* [WIP][event-hubs] EventHubBufferedProducerClient implementation (#18106)

* transfer

* [mock-hub] fix issue where calling stop() would not remove existing idle connection timeout watchers

* [event-hubs] reduce repetition in hubruntime.spec.ts tests

* Add support for flush,eventSuccess and eventError handlers, and backpressure to EventHubBufferedProducerClient

* add documentation and update API to align with .NET

* [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping (#18331)

* [Buffered Event Hubs Producer] Implements Parition Key to Partition ID mapping

* fix the implementation

* factor out the hashing logic

* remove unused import

* the test pass but the implementation needs to be simplified

* simplified implementation

* address feedback

* address feedback

* fix format

* address feedback

Co-authored-by: chradek <[email protected]>

* [Event Hubs] Prepare release (#18672)

* [Event Hubs] Prepare release

* remove empty sections

* Node doesn't support some samples for smoke test (#18496)

* Node doesn't support some samples for smoke test

* [search-documents] reprocessed samples with exp. generator

Co-authored-by: Will Temple <[email protected]>

* Add rlc quickstart guideline (#18503)

* add llc quickstart guideline

* update format

* add documentation about ci.yml

* update to resolve some comments

* update to resolve some comments

* update to resolve comments

* updates term

* update format

* update format

* Sync eng/common directory with azure-sdk-tools for PR 2265 (#18683)

* Improve devops logging for link checker

* Update eng/common/scripts/Verify-Links.ps1

Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>

* Build and test predefined set of packages only in identity pipelines (#18686)

* Build and test predefined set of packages only in identity pipelines

* Removed raw response from ContentDownloadResponse

* Changing name to RepeatanleContentDownloadResult

* Changed name to ContentDownloadResult

* Fixes after merging

Co-authored-by: Maor Leger <[email protected]>
Co-authored-by: Azure SDK Bot <[email protected]>
Co-authored-by: Ben Broderick Phillips <[email protected]>
Co-authored-by: scbedd <[email protected]>
Co-authored-by: Deyaaeldeen Almahallawi <[email protected]>
Co-authored-by: chradek <[email protected]>
Co-authored-by: Sarangan Rajamanickam <[email protected]>
Co-authored-by: Will Temple <[email protected]>
Co-authored-by: Qiaoqiao Zhang <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: Wes Haggard <[email protected]>
Co-authored-by: praveenkuttappan <[email protected]>
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.

4 participants