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

ProcessorOptions StartFromBeginning is not updating a Lease #84

Closed
CodeCheetah opened this issue Jun 18, 2018 · 14 comments
Closed

ProcessorOptions StartFromBeginning is not updating a Lease #84

CodeCheetah opened this issue Jun 18, 2018 · 14 comments

Comments

@CodeCheetah
Copy link

ChangeFeedProcessorOptions is not updating the lease.

ChangeFeedProcessorOptions feedProcessorOptions = new ChangeFeedProcessorOptions { StartFromBeginning = startFromBeginning };

@mkolt
Copy link
Contributor

mkolt commented Nov 19, 2018

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

@ealsur
Copy link
Member

ealsur commented Dec 11, 2018

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
Copy link

@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
Copy link
Member

ealsur commented Jan 16, 2019

@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
Copy link

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
Copy link

@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 ?

@bartelink
Copy link

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
Copy link

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
Copy link
Member

ealsur commented Jan 18, 2019

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

@bartelink
Copy link

OK, will do

  1. so are you saying the above text is broadly correct ?
  2. do you have any idea regarding your stance on How to force a complete restart of a projection #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
Copy link
Member

ealsur commented Jan 18, 2019

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
Copy link
Member

ealsur commented Jan 18, 2019

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.

@ealsur ealsur closed this as completed Jan 18, 2019
@bartelink
Copy link

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

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

No branches or pull requests

4 participants