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

Fix race that causes "lease id cannot be null or empty" illegal argument exception from storage #64

Merged
merged 2 commits into from
Mar 3, 2017

Conversation

JamesBirdsall
Copy link
Contributor

Problem occurs in acquireLease and is a race condition that requires multiple instances of EventProcessorHost and storage calls to be interleaved in a particular order.

When this instance of EventProcessorHost scanned the lease blobs, this partition was unowned (token is empty) but between then and now, another instance of EPH has established a lease (getLeaseState() is LEASED). We normally enforce that we only steal the lease if it is still owned by the instance which owned it when we scanned, but we can't do that when we don't know who owns it. The safest thing to do is just fail the acquisition. If that means that one EPH instance gets more partitions than it should, rebalancing will take care of that quickly enough.

Description

This checklist is used to make sure that common guidelines for a pull request are followed.

  • I have read the contribution guidelines.
  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR.
  • The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).
  • If applicable, the public code is properly documented.
  • Pull request includes test coverage for the included changes.
  • The code builds without any errors.

@msftclas
Copy link

@JamesBirdsall,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@serkantkaraca
Copy link
Member

serkantkaraca commented Mar 2, 2017

		throw new IllegalArgumentException("acquireLeaseSync: newLeaseId really is " + ((newLeaseId == null) ? "null" : "empty"));

James, I thought this was throwing, no? #Closed


Refers to: azure-eventhubs-eph/src/main/java/com/microsoft/azure/eventprocessorhost/AzureStorageCheckpointLeaseManager.java:394 in 50fb1db. [](commit_id = 50fb1db, deletion_comment = False)

@JamesBirdsall
Copy link
Contributor Author

No, that message did not appear in the logs. Also, if that line threw, then the storage call which did throw and did appear in the logs would not have been made.

}
else
{
newToken = leaseBlob.changeLease(newLeaseId, AccessCondition.generateLeaseCondition(lease.getToken()));
Copy link
Member

@serkantkaraca serkantkaraca Mar 2, 2017

Choose a reason for hiding this comment

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

lease.getToken() [](start = 92, length = 16)

Due to the same race, this can still throw NullRef, right? #Closed

@JamesBirdsall
Copy link
Contributor Author

getToken() does not fetch anything from Storage, it just returns the string currently stored in the Lease. That string does not change unless the instance is replaced or setToken() is called. In this case, all three calls to getToken() are guaranteed to return the same value.

@serkantkaraca
Copy link
Member

:shipit:

@JamesBirdsall JamesBirdsall merged commit 8d27787 into dev Mar 3, 2017
@JamesBirdsall JamesBirdsall deleted the javaEPHleaseid branch March 4, 2017 01:43
@JamesBirdsall JamesBirdsall added this to the 0.14.0 milestone Mar 23, 2017
SwayGom pushed a commit that referenced this pull request May 17, 2024
Fix race that causes "lease id cannot be null or empty" illegal argument exception from storage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants