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

PHOENIX-6871 Fix write threads deadlock #1559

Merged
merged 2 commits into from
Feb 13, 2023

Conversation

mnpoonia
Copy link
Contributor

@mnpoonia mnpoonia commented Feb 9, 2023

No description provided.

@mnpoonia
Copy link
Contributor Author

mnpoonia commented Feb 9, 2023

@tkhurana FYI

Copy link

@gourabtaparia gourabtaparia left a comment

Choose a reason for hiding this comment

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

One minor comment.

@@ -502,9 +503,10 @@ private void populateRowsToLock(MiniBatchOperationInProgress<Mutation> miniBatch
Mutation m = miniBatchOp.getOperation(i);
if (this.builder.isAtomicOp(m) || this.builder.isEnabled(m)) {
ImmutableBytesPtr row = new ImmutableBytesPtr(m.getRow());
if (!context.rowsToLock.contains(row)) {
// This seems to be unnecessary as we are using a set here
// if (!context.rowsToLock.contains(row)) {

Choose a reason for hiding this comment

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

Thanks for improving this - can you remove this commented code, as don't think this needs to be checked in.

Copy link
Contributor

@tkhurana tkhurana Feb 9, 2023

Choose a reason for hiding this comment

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

Thanks @mnpoonia . Looks good. Please remove the comments as suggested by @gourabtaparia

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@mnpoonia
Copy link
Contributor Author

mnpoonia commented Feb 9, 2023

Test failure

FailoverPhoenixConnectionIT.testConnectionWhenActiveZKRestarts

It is unrelated.

@@ -227,7 +228,7 @@ public static class BatchMutateContext {
// The collection of candidate index mutations that will be applied after the data table mutations
private ListMultimap<HTableInterfaceReference, Pair<Mutation, byte[]>> indexUpdates;
private List<RowLock> rowLocks = Lists.newArrayListWithExpectedSize(QueryServicesOptions.DEFAULT_MUTATE_BATCH_SIZE);
private HashSet<ImmutableBytesPtr> rowsToLock = new HashSet<>();
private TreeSet<ImmutableBytesPtr> rowsToLock = new TreeSet<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: private Set<ImmutableBytesPtr> rowsToLock = new TreeSet<>()

Copy link
Contributor Author

@mnpoonia mnpoonia Feb 10, 2023

Choose a reason for hiding this comment

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

Done

@mnpoonia
Copy link
Contributor Author

@gourabtaparia @tkhurana @virajjasani Please review and merge if changes look good

Copy link
Contributor

@gjacoby126 gjacoby126 left a comment

Choose a reason for hiding this comment

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

+1, thanks @mnpoonia

@tkhurana tkhurana merged commit c70429f into apache:master Feb 13, 2023
@virajjasani
Copy link
Contributor

Belated +1, thank you @mnpoonia!

@mnpoonia
Copy link
Contributor Author

mnpoonia commented Mar 1, 2023

@tkhurana @virajjasani Can you cherrypick to 5.1 branch as well.

virajjasani pushed a commit that referenced this pull request Mar 2, 2023
* PHOENIX-6871 Fix write threads deadlock

* Add  comment about Treeset importance
@virajjasani
Copy link
Contributor

Done @mnpoonia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants