[Merge]PublicAPIAndMigration#32187
Conversation
ghost
added
the
| * @return current Builder. | ||
| */ | ||
| @Beta(value = Beta.SinceVersion.V4_40_0, warningText = Beta.PREVIEW_SUBJECT_TO_CHANGE_WARNING) | ||
| public ChangeFeedProcessorBuilder handleLatestVersionChanges(Consumer<List<JsonNode>> consumer) { |
There was a problem hiding this comment.
should we return JsonNode or ChangeFeedProcessorItem
There was a problem hiding this comment.
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.
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
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
xinlian12
requested review from
FabianMeiswinkel,
Pilchie,
aayush3011,
jeet1995,
kirankumarkolli,
kushagraThapar,
milismsft and
simorenoh
as code owners
| private final Logger logger = LoggerFactory.getLogger(BootstrapperImpl.class); | ||
| private final PartitionSynchronizer synchronizer; | ||
| private final LeaseStore leaseStore; | ||
| private final LeaseStoreManager pkVersionLeaseStoreManager; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
make sense, maybe pkRangeIdVersion?
|
|
||
| @Override | ||
| public Mono<Void> delete(List<Lease> leases) { | ||
| throw new UnsupportedOperationException("Delete with leases are not supported for Change Feed epk version wire format"); |
There was a problem hiding this comment.
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...
| private final ChangeFeedObserverFactory<JsonNode> observerFactory; | ||
| private volatile String databaseResourceId; | ||
| private volatile String collectionResourceId; | ||
| private volatile String databaseId; |
There was a problem hiding this comment.
Let's discuss in the design review - but this IMO is a bug. You need resource ids to identify deleted + recreated collections.
There was a problem hiding this comment.
yes it is -> the question will be whether we want to change in the old change feed processor version. And when we change, migration will need to happen again
but on a further thought, this bug actually could very well blocking the migration story in the new lease store, because how we gonna know we are migration the correct ones
|
|
||
| return this.leaseDocumentClient.executeBulkOperations(Flux.defer(() -> Flux.fromIterable(operations))) | ||
| .flatMap(itemResponse -> { | ||
| if (itemResponse.getResponse() != null && itemResponse.getResponse().isSuccessStatusCode()) { |
There was a problem hiding this comment.
Can't that run into a hnag very easily. delete being called concurrently could result in operations being added for docs that might not exist anymore - 404 would not satisfy isSuccessStatusCode and you'll end-up in an endless loop in operations.size != 0?
There was a problem hiding this comment.
you are right Fabian, I need to add more handling around it -- want to get some feedback around the delete leases part in the meeting today
|
closed this one as different PR will be created for public API(Discussed and agreed with team) and Migration (more discussion will be followed) |
3 participants
Description:
Part of the effort of adding Merge support in Java SDK. Main working item: #32131
Changes
x-ms-cosmos-sdk-supported-capabilities= 1 in the headerpkversionreadFeed request -x-ms-cosmos-sdk-supported-capabilities= 0 aspkversionchange feed processor implementation does not support mergehandleLatestVersionChanges(Consumer<List<ChangeFeedProcessorItem>> consumer)