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.

Sign up for GitHub

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

Jump to bottom

[ADDED] deterministic subject tokens to partition mapping #2890

Merged
merged 8 commits into from
Mar 25, 2022
Merged

[ADDED] deterministic subject tokens to partition mapping #2890

merged 8 commits into from
Mar 25, 2022

Conversation

jnmoyne
Copy link
Contributor

@jnmoyne jnmoyne commented Mar 2, 2022 •
edited

introduces 'Moustache' style subject mapping format (e.g. foo.. -> foo.{{wildcard(1)}}.{{wildcard(2)}}.{{partition(10,1,2)}})

  • Tests added
  • Branch rebased on top of current main (git pull --rebase origin main)
  • Changes squashed to a single commit (described here)
  • Build is green in Travis CI
  • You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

/cc @nats-io/core

@jnmoyne
Adds deterministic subject tokens to partition mapping
79fd359 
introduces 'Moustache' style subject mapping format (e.g. foo.*.* -> foo.{{wildcard(1)}}.{{wildcard(2)}}.{{partition(10,1,2)}})
@jnmoyne
Make staticcheck happy (https://staticcheck.io/docs/checks#S1007)
c4e03a2
@jnmoyne
Adds deterministic subject tokens to partition mapping
7fa088c 
introduces 'Moustache' style subject mapping format (e.g. foo.*.* -> foo.{{wildcard(1)}}.{{wildcard(2)}}.{{partition(10,1,2)}})
@jnmoyne
Merge remote-tracking branch 'origin/jnm/partition_mapping' into jnm/…
ab0853f 
…partition_mapping
kozlovic
kozlovic requested changes Mar 2, 2022
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

First pass at the review.

server/accounts.go Outdated Show resolved Hide resolved
server/accounts.go Outdated Show resolved Hide resolved
server/accounts.go Outdated Show resolved Hide resolved
@jnmoyne
Change mapping function name being passed as a string to being passed…
ceb47f0 
… as a 'constant' regex and small simplification because the hash's Write never returns an error
matthiashanel
matthiashanel suggested changes Mar 3, 2022
View reviewed changes
server/accounts.go Outdated Show resolved Hide resolved
server/accounts.go Outdated Show resolved Hide resolved
server/accounts.go Outdated Show resolved Hide resolved
server/accounts.go Outdated Show resolved Hide resolved
server/accounts.go Outdated Show resolved Hide resolved
@jnmoyne
Copy link
Contributor Author

jnmoyne commented Mar 4, 2022

All requested changes implemented by latest commit

ripienaar
ripienaar requested changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

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

looking good, some small comments

server/accounts.go Outdated Show resolved Hide resolved
server/accounts.go Show resolved Hide resolved
server/accounts.go Show resolved Hide resolved
server/accounts_test.go Show resolved Hide resolved
@ripienaar ripienaar self-requested a review March 11, 2022 09:42
ripienaar
ripienaar approved these changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

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

LGTM, but you'll definitely have to get ivan|derek|mh to approve also

@ripienaar ripienaar mentioned this pull request Mar 16, 2022
Closed
kozlovic
kozlovic approved these changes Mar 25, 2022
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@kozlovic kozlovic changed the title Adds deterministic subject tokens to partition mapping [ADDED] deterministic subject tokens to partition mapping Mar 25, 2022
derekcollison
derekcollison approved these changes Mar 25, 2022
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@derekcollison derekcollison merged commit 7e4a4c8 into main Mar 25, 2022
@derekcollison derekcollison deleted the jnm/partition_mapping branch March 25, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ripienaar ripienaar ripienaar approved these changes

@derekcollison derekcollison derekcollison approved these changes

@kozlovic kozlovic kozlovic approved these changes

@matthiashanel matthiashanel Awaiting requested review from matthiashanel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jnmoyne @ripienaar @derekcollison @kozlovic @matthiashanel