Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Custom endpoint_url and 'ShardId' #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nicholasball
Copy link

  • Add custom endpoint_url to consumer/producer and state
  • Return 'ShardId' in item

Copy link

@dmrib dmrib left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@dmrib
Copy link

dmrib commented Jan 6, 2021

Is there anything I can do to move this forward? I really need this PR merged :)

Copy link
Contributor

@borgstrom borgstrom left a comment

Choose a reason for hiding this comment

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

Hi @nicholasball,

I'm 👍 on the end point change, but would like to change the semantics of how the shard_id is returned to keep the data envelope stable for existing use cases.

We also will need a test that covers return_shard_id.

Comment on lines +204 to 206
item['ShardId']=shard_id
log.debug(item)
yield item
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like the idea mutating the item.

Instead of adding the ShardId to the item what do you think about adding another option to init (something like return_shard_id=False) that if true would change the yield to:

if self.return_shard_id:
    yield shard_id, item
else:
    yield item

@borgstrom borgstrom self-assigned this Feb 1, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants