Skip to content
This repository was archived by the owner on Mar 24, 2021. It is now read-only.

Conversation

emmettbutler
Copy link
Contributor

for #253

Copy link

Choose a reason for hiding this comment

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

I think you may want to recompute this within your retry loop since owned_partition_offsets is mutated by the loop.

@emmettbutler emmettbutler force-pushed the bugfix/ordered_partitions branch from 881265a to 5a1b071 Compare October 1, 2015 18:53
@emmettbutler emmettbutler added this to the 2.0.1 milestone Oct 1, 2015
@emmettbutler emmettbutler self-assigned this Oct 1, 2015
@emmettbutler
Copy link
Contributor Author

@yungchin This could use a sanity check.

@emmettbutler emmettbutler assigned yungchin and unassigned emmettbutler Oct 1, 2015
@yungchin
Copy link
Contributor

yungchin commented Oct 2, 2015

@emmett9001 is the sorting in fetch() strictly necessary? As that code just skips over any partitions it cannot grab a lock on, doesn't that mean there shouldn't be an issue with deadlocks there? Other than that, this looks good to me.

@emmettbutler
Copy link
Contributor Author

No, I don't think it's strictly necessary. I figured sorting is a good idea in general because it makes the whole system more predictable.

@yungchin
Copy link
Contributor

yungchin commented Oct 2, 2015

Ok, yeah that seems fine to me - I don't imagine the sorting is very expensive.

I've not been able to reproduce the bug here, so can't confirm that it's fixed, but I'm convinced that sorting before taking all the locks is a very real improvement either way.

emmettbutler added a commit that referenced this pull request Oct 2, 2015
enforce id-based ordering before attempting to obtain partition locks.
@emmettbutler emmettbutler merged commit 1b3cc80 into master Oct 2, 2015
@emmettbutler emmettbutler deleted the bugfix/ordered_partitions branch October 2, 2015 21:55
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.

5 participants