Azure / azure-documentdb-changefeedprocessor-dotnet

This library provides a host for distributing change feed events in partitioned collection across multiple observers. Instances of the host can scale up (by adding) or down (by removing) dynamically, and the load will be automatically distributed among active instances in about-equal way.
Other
51 stars 22 forks source link

ProcessorOptions StartFromBeginning is not updating a Lease #84

Closed CodeCheetah closed 5 years ago

CodeCheetah commented 6 years ago

ChangeFeedProcessorOptions is not updating the lease.

ChangeFeedProcessorOptions feedProcessorOptions = new ChangeFeedProcessorOptions { StartFromBeginning = startFromBeginning };

mkolt commented 5 years ago

Please provide more details about this issue (it's not clear what it's about), in particular, expected and actual behavior.

ealsur commented 5 years ago

If I understand correctly, StartFromBeginning will only apply if there are no Leases (or Leases have no Continuation Token). I believe this behavior actually is correct, because let's say you start it with StartFromBeginning and then stop it after a while. You'd expect that it would pick up from the last known checkpoint without needing to change the settings (ie remove the StartFromBeginning flag).

bartelink commented 5 years ago

@ealsur I'm not sure I understand your point - even if I was using StartFromBeginning=true to be explicit about the basis on which I want to configure a lease the first time a) that's the default b) it doesn't get persisted AFAICT

In other words, AIUI, there is no practical use for this property. If I'm correct, either the xmldoc is misleading or this feature needs to be deprecated.

@CodeCheetah please provide mode context or close this issue (I think #123 is a duplicate of what you're asking, but nobody will ever know for sure unless you respond)

ealsur commented 5 years ago

@bartelink The property works as expected.

  1. Make sure your Leases collection is empty
  2. Do some inserts/updates in the Monitored collection
  3. Set the StartFromBeginning flag to true and start the Processor
  4. The processor will read the changes done in 3 even though they happened before it was ever started
  5. It will update the Leases with the latest checkpoint, that is, the point in time after 4
  6. If you stop and start the Processor, since there are Leases in place, it will read their state and start from that point in time (4).

The flag works as expected, it is meant to be used on a first-start scenario (no Leases). Afterwards, the Processor picks up the last saved state if it gets stopped/started.

bartelink commented 5 years ago

Thanks @ealsur, that makes sense; I will verify.

It does prompt the following for me:

  1. The xmldocs are not clear about this (it seems this OP fell into the same trap) -> Can we have some more explicit text in there please?
  2. I'm definitely still looking for an answer to 123 if this is the case
bartelink commented 5 years ago

@ealsur It seems (not doubting you, but wanted to verify) that you are correct @CodeCheetah From your perspective, given the above, would you be happy to close this as soon as the StartTime/StartFromBeginning/StartContinuation xmldocs are clear that these are solely about how a new lease collection is seeded ?

ealsur commented 5 years ago

@bartelink The links also explain the same behavior: https://docs.microsoft.com/en-us/dotnet/api/microsoft.azure.documents.changefeedprocessor.changefeedprocessoroptions.startfrombeginning?view=azure-dotnet#Microsoft_Azure_Documents_ChangeFeedProcessor_ChangeFeedProcessorOptions_StartFromBeginning

image

bartelink commented 5 years ago

I agree your citation is not stating any untruths. I also agree that, especially given one has read your clear response above, one is unlikely to misinterpret what it means.

The problem is that I and the OP appear to have made the same misinterpretation of the docs.

I'm saying that the docs should be reworded to make things more explicit - i.e. the docs are not failing a test, but they need to be refactored for clarity ;)

(I'll see if I can craft something and post shortly...)

bartelink commented 5 years ago

The current wording can be misinterpreted to imply that given some complex set of poking in values that it can be made to do your bidding; your much less ambiguous explanation above makes it clear that this is incorrect.

I'm suggesting that each of the three options have consistent wording added to underscore that a) they are mutually exclusive b) they only apply to uninitialized lease collections

<summary>
Gets or sets a flag specifying (for an uninitialized lease collection) whether processing should only traverse changes from the present position forward (`false`, default), or should traverse all documents in the collection first (when `true`)<br/>
NB This setting is only considered when no lease collection with a valid continuation token has been persisted in the lease store. <br/>
Where multiple options are specified, they will all be accepted, but are mutually exclusive. The order of precedence is 1) `StartTime` 2) `StartContinuation` 3) `StartFromBeginning`
</summary>

Ideally, given a resolution of #123, the text can also make it more explicit by replacing

NB This setting is only considered when no lease collection with a valid continuation token has been persisted in the lease store.

with

NB This setting is only considered when a) ReinitializeCollection has been set to true OR b) a lease collection with a valid continuation token is not present in the lease store.

ealsur commented 5 years ago

Great suggestion! We can certainly review it if you want to send a PR with the text you consider better 😄

bartelink commented 5 years ago

OK, will do

  1. so are you saying the above text is broadly correct ?
  2. do you have any idea regarding your stance on #123 yet ? the comment would likely refer to that (TL;DR as evidenced by my need, and the OP's misinterpretation, presumably based on the same need/desire, wanting to completely kill and hence restart a given projection is something that people want (ot think they want)) - Is it possible to rule in or out adding such a facility in some manner ?
ealsur commented 5 years ago

I agree with the text correction but any PR will be reviewed by the complete team 😄 Regarding #123 I'm not familiar with the reason for the original setting to be marked as obsolete or unsupported, will let the team comment on that on that separate thread

ealsur commented 5 years ago

Closing the issue as the behavior is as expected, in case that it's not behaving correctly with an empty leases collection, it can be reopened for investigation.

bartelink commented 5 years ago

Thanks - and thanks for closing ! I'll do the PR when 123 reaches a conclusion as the present text definitely confused the heck out of me, both in terms of navigating the builder via intellisense and trying to infer the logic from the impl