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

NATS PubSub - adds ability to distinguish between different Publish requests #59

Merged
merged 5 commits into from Jun 11, 2019

Conversation

Projects
None yet
2 participants
@aaslamin
Copy link
Contributor

commented Jun 4, 2019

Parent issue: aporeto-inc/aporeto#1274

Context

This PR adds the ability to distinguish between different Publish requests when using the NATS request-reply messaging pattern. Our existing PubSubOptPublish options are impacted as follows:

NATSOptPublishRequireAck option


When configured, it will set the ResponseMode field of the Publication to “ReplyWithACK”. The side effect of this is that caller of Publish will now block until an ACK is received or the supplied context deadline is reached. As a result of this option our implementation of Subscribe will now need to look for this new ResponseMode attribute when consuming messages to determine whether it needs to respond back with an ACK - the value would be set to “ReplyWithACK”. This option is mutually exclusive with the NATSOptRespondToChannel option.

NATSOptRespondToChannel option


When configured, it will set the ResponseMode field of the Publication to “ReplyWithPublication”. The side effect of this is that caller of Publish will now block until a Publication response is received or the supplied context deadline is reached. As a result of this option our implementation of Subscribe will now need to look for the new ResponseMode attribute and check to see if its value is set to “ReplyWithPublication” as this would imply that a Publication response should be delivered AFTER processing has been completed. This option is mutually exclusive with the NATSOptPublishRequireAck option.

⚠️ Breaking change(s)

  • removed the NATSOptPublishReplyValidator publish option as it now serves no purpose. Folks should either use NATSOptPublishRequireAck or NATSOptRespondToChannel now. As a result, the replyValidator attribute from the publish configuration has been removed as well.

If merged, will this break 💥 existing consumers?

This changes introduced here should not break™ existing clients after they upgrade their Bahamut package (asides for the breaking change mentioned above). The existing Subscribe implementation should work as is because currently it is just checking whether the received raw *nats.Msg type it gets has a Reply attribute set. If it does, it uses the value in that field as the subject to send back an ACK reply to.

Coming next

Modify our Subscribe implementation to distinguish between the expected ResponseMode which is now serialized into the Publication. The Subscribe implementation should check this field and behave accordingly:

  • If the expected response mode is an ACK, it should reply back straight away before processing the publication
  • If the expected response mode is a Publication, it should process the received publication first and then respond back with another publication. Perhaps the client code can utilize a new convenience method here on the Publication type? e.g. "Reply" or something similar.
new: adds ability to distinguish between different Publish requests w…
…hen using the NATS request-reply messaging

• NATSOptPublishRequireAck option

When configured, it will set the ResponseMode field of the Publication to “ReplyWithACK”. The side effect of this is that caller of Publish will now block until an ACK is received or the supplied context deadline is reached. As a result of this option our implementation of Subscribe will now need to look for this new ResponseMode attribute when consuming messages to determine whether it needs to respond back with an ACK - the value would be set to “ReplyWithACK”. This option is mutually exclusive with the NATSOptRespondToChannel option.

• NATSOptRespondToChannel option

When configured, it will set the ResponseMode field of the Publication to “ReplyWithPublication”. The side effect of this is that caller of Publish will now block until a Publication response is received or the supplied context deadline is reached. As a result of this option our implementation of Subscribe will now need to look for the new ResponseMode attribute and check to see if its value is set to “ReplyWithPublication” as this would imply that a Publication response should be delivered AFTER processing has been completed. This option is mutually exclusive with the NATSOptPublishRequireAck option.

breaking: removed the NATSOptPublishReplyValidator publish option as it now serves no purpose. Folks should either use NATSOptPublishRequireAck or NATSOptRespondToChannel now. As a result, the replyValidator attribute from the publish configuration has been removed as well.
Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

TODO:

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "59",
    "commit-sha": "670d21d3f1ecf071592934ebb52a518feea679ab"
  }
]
@codecov

This comment has been minimized.

Copy link

commented Jun 4, 2019

Codecov Report

Merging #59 into master will decrease coverage by 0.04%.
The diff coverage is 91.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   88.27%   88.22%   -0.05%     
==========================================
  Files          28       28              
  Lines        2124     2149      +25     
==========================================
+ Hits         1875     1896      +21     
- Misses        221      225       +4     
  Partials       28       28
Impacted Files Coverage Δ
pubsub_nats.go 55.55% <100%> (+3.7%) ⬆️
publication.go 96.42% <83.33%> (-3.58%) ⬇️
pubsub_nats_options.go 95.83% <90%> (-4.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f8df41...bb6088d. Read the comment docs.

Show resolved Hide resolved publication.go Outdated
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Question: should we panic if both NATSOptPublishRequireAck & NATSOptRespondToChannel are set?

I can add it in, but right now if someone were to do that it would just use the last option value to override the requestMode. It is still clearly an incorrect usage of the package, but nothing should blow up. Probably better to panic yeah to let the client code know they screwed up?

new: add test coverage for scenario: should return an error if respon…
…se is not a valid ACK response when using the NATSOptPublishRequireAck option

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "59",
    "commit-sha": "1c0f0b97a7ad100da10f714b6120afd5b1bd725e"
  }
]
@primalmotion
Copy link
Member

left a comment

After the 2 small things, lgtm

Show resolved Hide resolved publication.go Outdated
Show resolved Hide resolved publication.go Outdated
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Question: should we panic if both NATSOptPublishRequireAck & NATSOptRespondToChannel are set?

I can add it in, but right now if someone were to do that it would just use the last option value to override the requestMode. It is still clearly an incorrect usage of the package, but nothing should blow up. Probably better to panic yeah to let the client code know they screwed up?

@primalmotion do you have any opinion on 🔝? Is it better to panic or return an error if caller supplies more than one request mode option? I am not 💯% sure on the "Go way" for handling this.

My current implementation deals with this gracefully as the last option that is set will override whatever value requestMode currently has. So for example, if someone calls Publish like this:

ps.Publish(myPub,
NATSOptPublishRequireAck(context.Background()), 
NATSOptRespondToChannel(context.Background(), respCh))

Then the request mode will be set to requestModePublication as the last option provided was NATSOptRespondToChannel.

Although that would clearly be a code smell in the client code.

edit: update constant names to follow idiomatic convention to aid in …
…code completion

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
@primalmotion

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Question: should we panic if both NATSOptPublishRequireAck & NATSOptRespondToChannel are set?

I can add it in, but right now if someone were to do that it would just use the last option value to override the requestMode. It is still clearly an incorrect usage of the package, but nothing should blow up. Probably better to panic yeah to let the client code know they screwed up?

yes we should panic

new:
- panic if multiple options have been provided to set the request mode as this is clear logical error by the package consumer
    - new unit test coverage added to cover this behavioural change
- add default values for requestMode and ResponseMode
- add String method for requestMode type so when printing the constant, a human readable value will be printed instead of its literal integer value

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>

@aaslamin aaslamin requested a review from primalmotion Jun 10, 2019

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@primalmotion I think I am ready for potentially a final review 🙏.

My last commit:

  • added default values for requestMode & ResponseMode constants
  • 💥 panics if more than one option has been provided to set the requestMode property
    • new unit test coverage for 🔝 behavioural change
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "59",
    "commit-sha": "c3e40846fcf3b936a8e1729403641e0669c57d9f"
  }
]
Show resolved Hide resolved publication.go
Show resolved Hide resolved pubsub_nats_options.go Outdated
Show resolved Hide resolved pubsub_nats_options.go Outdated
Show resolved Hide resolved pubsub_nats_options.go Outdated
responseCh chan *Publication
ctx context.Context
requestMode requestMode
responseCh chan *Publication
}

// NATSOptSubscribeQueue sets the NATS subscriber queue group.

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jun 10, 2019

Author Contributor

I haven't, it's still there. 5 lines below to be exact :D

responseCh chan *Publication
ctx context.Context
requestMode requestMode
responseCh chan *Publication
}

// NATSOptSubscribeQueue sets the NATS subscriber queue group.

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jun 10, 2019

Author Contributor

I haven't, it's still there. 5 lines below to be exact :D

Show resolved Hide resolved pubsub_nats_options.go Outdated
edit:
- re-use ResponseMode constants for publish config
- panic if options were called with nil channel or context
  - add test coverage to ensure panic occurs if option is provided illegally

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "59",
    "commit-sha": "bb6088dee129c6292d0a3344156afae1bf85432f"
  }
]

@aaslamin aaslamin requested a review from primalmotion Jun 10, 2019

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Update - latest commit:

  • Now reusing ResponseMode constants for publish config
  • 💥 panic if options were called with nil channel or context
    • add test coverage to ensure panic occurs if option is provided illegally
Show resolved Hide resolved pubsub_nats.go
@primalmotion
Copy link
Member

left a comment

Please run a full ft

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "bahamut-pubsub-publish",
    "component": "bahamut",
    "pr-id": "59",
    "commit-sha": "bb6088dee129c6292d0a3344156afae1bf85432f"
  },
  {
    "project": "bahamut-pubsub-publish",
    "component": "backend",
    "pr-id": "425",
    "commit-sha": "6410d32247478ae7efa2cfac1f8691a54f896350"
  }
]
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Functional tests are running now - will check back results tomorrow 🙏

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Looks like we're good to go 🎉

@aaslamin aaslamin merged commit 16be4e7 into master Jun 11, 2019

6 checks passed

built
Details
codecov/patch 91.48% of diff hit (target 88.27%)
Details
codecov/project Absolute coverage decreased by -0.04% but relative coverage increased by +3.21% compared to 9f8df41
Details
functional-tests Submitter: reason: . functional-tests set to success
Details
functional-tests-trigger Submitter: reason: . functional-tests-trigger set to success
Details
unit-tests
Details

@aaslamin aaslamin deleted the distinguish-between-ack-reply-requests branch Jun 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.