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

feat(post-process-forwarder) Different errors and transaction forwarders #28954

Merged
merged 2 commits into from
Oct 1, 2021

Conversation

nikhars
Copy link
Member

@nikhars nikhars commented Sep 29, 2021

Added different Errors and Transactions post process forwarders. The
errors one handles backward compatibility use case as well where the
kafka header might be missing. These new classes aren't going to be
used right now. They will be integrated in a different PR.

Added different Errors and Transactions post process forwarders. The
errors one handles backward compatibility use case as well where the
kafka header might be missing. These new classes aren't going to be
used right now. They will be integrated in a different PR.
@nikhars nikhars requested a review from a team as a code owner September 29, 2021 18:12
Copy link
Contributor

@fpacifici fpacifici left a comment

Choose a reason for hiding this comment

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

I don't think you need to use mocks for the Message objects in your tests. Other than that no issue with this.

forwarder = PostProcessForwarderWorker(concurrency=1)
@pytest.fixture
def kafka_message_without_transaction_header(kafka_message_payload):
mock_message = Mock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using a mock for the message? Can't we just create a real Message object ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried using actual confluent_kafka.Message but it does not work. For example the set_headers expect Message objects rather than the actual headers. When I create a Message and pass it to set_headers, it barfs saying "cannot create 'cimpl.Message' instances".

I feel its fine to use mocks here, since they return values which would have been returned by the actual message when the api's like headers(), values() are called.

@nikhars nikhars merged commit c2e5d42 into master Oct 1, 2021
@nikhars nikhars deleted the nikhar/separate_post_process_forwarders branch October 1, 2021 19:49
nikhars added a commit that referenced this pull request Oct 15, 2021
…rwarders (#29225)

In feat(post-process-forwarder) Different errors and transaction forwarders #28954, we created different processing strategies for errors and transactions post process forwarders.
In this PR, we enable post process forwarder to run with one of the following entities (errors, transactions, all)
The all option would be used in scenarios where we don't need separate post process forwarders. Example devserver, single tenant.
There is no production change with this code since the default option for entity is "all" which means the current post process forwarder would handle all messages
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants