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

HollowIncrementalProducer - added restoreFromLastState [proposal] #181

Merged

Conversation

@rpalcolea
Copy link
Member

commented Mar 3, 2018

This pull request:

  • Introduces Builder pattern for HollowIncrementalProducer
  • Users could provide AnnouncementWatcher, BlobRetriever and data model
  • Users can now restore from last state using the HollowIncrementalProducer.restoreFromLastState).
  • Kept public constructors: these could be removed later. I know HollowIncrementalProducer is a BETA API but for now I think it makes sense to keep them and perhaps deprecate in Hollow 3?

With the introduction of #178 and this, users could now write the first cycle using the incremental directly and restore from previous states. This would significantly reduce their code by avoiding all that logic that basically everyone would have to write in order to restore an incremental producer.

In the future HollowIncrementalProducer could be just another type of HollowProducer but that's not in the scope of this pull request.

Roberto Perez Alcolea

@rpalcolea rpalcolea changed the title added restoreFromLastState [proposal] HollowIncrementalProducer - added restoreFromLastState [proposal] Mar 5, 2018

Roberto Perez Alcolea
@rpalcolea

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2018

Hi @toolbear , please let me know your thoughts on this

@duro1

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2018

@rpalcolea This looks good. It currently is a little involved to know how and in what order to initialize the classic producer in order to properly use the incremental producer.
What if instead of a builder, the newly proposed constructor arguments were mandatory, in the next major release?

This would allow the client to call the restoreFromLastState at any point, but know that the necessary objects should be present. This would also allow for controls to be added around running the first cycle only after restoring (or verifying a previous does not exist)

e.g.
private boolean isInitialized;

restoreFromLastState(...)
    (attempt restore)
    isInitialized=true;

public long runCycle() {
    if(!isInitialized)
        throw new IllegalStateException("You must restore state prior running an IncrementalProducer cycle");
    ...

@toolbear toolbear merged commit 8e5f4f5 into Netflix:master Apr 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rpalcolea rpalcolea deleted the rpalcolea:incremental-producer/add-restore-support branch Apr 24, 2018

Sunjeet pushed a commit that referenced this pull request Apr 10, 2019
Merge pull request #181 in PDT/cinder from fix/multi-token-namespace …
…to master

* commit '5af2a4e6ee01aef29534dd758d1eca4bd1aac6fd':
  Handle multi token namespace e.g. streamster_builds_smoketest_v0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.