-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[Merge]PublicAPIAndMigration #32187
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
[Merge]PublicAPIAndMigration #32187
Changes from all commits
ccfe075
7d96023
8bff83f
ee9156c
a37668a
b940194
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import com.azure.cosmos.implementation.CosmosSchedulers; | ||
| import com.azure.cosmos.implementation.changefeed.Bootstrapper; | ||
| import com.azure.cosmos.implementation.changefeed.LeaseStore; | ||
| import com.azure.cosmos.implementation.changefeed.LeaseStoreManager; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import reactor.core.publisher.Mono; | ||
|
|
@@ -21,20 +22,28 @@ class BootstrapperImpl implements Bootstrapper { | |
| private final Logger logger = LoggerFactory.getLogger(BootstrapperImpl.class); | ||
| private final PartitionSynchronizer synchronizer; | ||
| private final LeaseStore leaseStore; | ||
| private final LeaseStoreManager pkVersionLeaseStoreManager; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know the term "pkVersion" comes from a previous PR - so non-blocking. I got confused by this prefix - pk - assuming stands for partition key - what is meant here is PKRangeId (PartitionKeyRangeId) - i was wondering why you want a logical partition based lease store - until I realized pk stands for partitioneyrangeid not partitionkey. First thought it is a typo missing the e of epk :-) - anyway like mentioned non-blocking but IMO not very intuitive.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sense, maybe pkRangeIdVersion?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. much better IMO |
||
| private final Duration lockTime; | ||
| private final Duration sleepTime; | ||
|
|
||
| private volatile boolean isInitialized; | ||
| private volatile boolean isLockAcquired; | ||
|
|
||
| public BootstrapperImpl(PartitionSynchronizer synchronizer, LeaseStore leaseStore, Duration lockTime, Duration sleepTime) { | ||
| public BootstrapperImpl( | ||
| PartitionSynchronizer synchronizer, | ||
| LeaseStore leaseStore, | ||
| Duration lockTime, | ||
| Duration sleepTime, | ||
| LeaseStoreManager pkVersionLeaseStoreManager) { | ||
|
|
||
| checkNotNull(synchronizer, "Argument 'synchronizer' can not be null"); | ||
| checkNotNull(leaseStore, "Argument 'leaseStore' can not be null"); | ||
| checkArgument(lockTime != null && this.isPositive(lockTime), "lockTime should be non-null and positive"); | ||
| checkArgument(sleepTime != null && this.isPositive(sleepTime), "sleepTime should be non-null and positive"); | ||
|
|
||
| this.synchronizer = synchronizer; | ||
| this.leaseStore = leaseStore; | ||
| this.pkVersionLeaseStoreManager = pkVersionLeaseStoreManager; | ||
| this.lockTime = lockTime; | ||
| this.sleepTime = sleepTime; | ||
|
|
||
|
|
@@ -66,7 +75,18 @@ public Mono<Void> initialize() { | |
| logger.info("Another instance is initializing the store"); | ||
| return Mono.just(isLockAcquired).delayElement(this.sleepTime, CosmosSchedulers.COSMOS_PARALLEL); | ||
| } else { | ||
| return this.synchronizer.createMissingLeases() | ||
| return this.shouldBootstrapFromPkVersionLeases() | ||
| .flatMap(shouldBootstrapFromPkVersion -> { | ||
| if (shouldBootstrapFromPkVersion) { | ||
| return this.pkVersionLeaseStoreManager.getAllLeases() | ||
| .collectList() | ||
| .flatMap(pkVersionLeases -> { | ||
| return this.synchronizer.createMissingLeases(pkVersionLeases) | ||
| .then(this.pkVersionLeaseStoreManager.delete(pkVersionLeases)); // after bootstrapping, we are going to delete all th pk version leases | ||
| }); | ||
| } | ||
| return this.synchronizer.createMissingLeases(); | ||
| }) | ||
| .then(this.leaseStore.markInitialized()); | ||
| } | ||
| }) | ||
|
|
@@ -85,4 +105,8 @@ public Mono<Void> initialize() { | |
| .repeat(() -> !this.isInitialized) | ||
| .then(); | ||
| } | ||
|
|
||
| private Mono<Boolean> shouldBootstrapFromPkVersionLeases() { | ||
| return this.pkVersionLeaseStoreManager == null ? Mono.just(false) : this.pkVersionLeaseStoreManager.isInitialized(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
|
|
||
| import java.time.Duration; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| import static com.azure.cosmos.implementation.guava25.base.Preconditions.checkArgument; | ||
| import static com.azure.cosmos.implementation.guava25.base.Preconditions.checkNotNull; | ||
|
|
@@ -205,6 +206,11 @@ public Mono<Void> delete(Lease lease) { | |
| .then(); | ||
| } | ||
|
|
||
| @Override | ||
| public Mono<Void> delete(List<Lease> leases) { | ||
| throw new UnsupportedOperationException("Delete with leases are not supported for Change Feed epk version wire format"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On first glance I would ask why you even added this to the base class? Handling UnsupportedException to recognize seems worse than letting compiler to pick it up? But maybe further review will help to answer that question... |
||
| } | ||
|
|
||
| @Override | ||
| public Mono<Lease> acquire(Lease lease) { | ||
| if (lease == null) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we return
JsonNodeorChangeFeedProcessorItemThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChangeFeedProcessorItem (to normalizer eventually on public surface area) - but to be honest I don't particularly like the name of that class (probably too late) - ChangeFeedNotification / event/ record would probably describe better what it is. Has the current beta-api naming been discussed with central team already? Or will we have the naming discussion anyway with them? If the former definitely ignore my feedback (not worth changing now). Even if we have not discussed it yet I would only consider it if there are others with similar feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to add a comment that this method is a replacement for the old handleChanges (that will eventually be deprecated) - but that the semantics are compatible