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

Remove Deprecated code of Events V1 #2634

Merged
merged 24 commits into from
Oct 11, 2019
Merged

Conversation

longquanzheng
Copy link
Collaborator

@longquanzheng longquanzheng commented Oct 4, 2019

One year ago, I introduced History Events V2 with more functionality(forking branches) and performance(removing LWT). However we had to keep V1 running for backward compatibility. After one year, we have found that there is no more workflows running with V1. It's time to remove the V1 code and the logic of backward compatibility.

TODO:

  1. Rename V2 interface to get rid of "V2" since there is no V1 anymore.
  2. Make SetHistoryTree transparent in mutableStateBuilder to make it more easy to maintain.

Copy link
Contributor

@yycptt yycptt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The archival part looks good to me.

The archival history iterator test uses mock of historyManagerV1 interface and the test is quite complicated. Let me create a separate diff to change it to use historyV2 so your test can pass.

@wxing1292
Copy link
Contributor

@yycptt if possible, could you use the paging iterator?
link

@yycptt
Copy link
Contributor

yycptt commented Oct 10, 2019

@wxing1292 I think they are a little bit different. The history iterator used by archiver needs the ability to get current state of the iterator and create a new iterator based on some previous states. I don't think the paging iterator supports that. The functionality is needed because archiver workflow needs to periodically record its current progress and resume from last saved state if worker restarts.

There are some other issues like keeping current iterator state when error occurs, performing lookahead for nextPageToken etc. If you want, we can talk about it in more detail offline.

@wxing1292
Copy link
Contributor

@wxing1292 I think they are a little bit different. The history iterator used by archiver needs the ability to get current state of the iterator and create a new iterator based on some previous states. I don't think the paging iterator supports that. The functionality is needed because archiver workflow needs to periodically record its current progress and resume from last saved state if worker restarts.

There are some other issues like keeping current iterator state when error occurs, performing lookahead for nextPageToken etc. If you want, we can talk about it in more detail offline.

If that is the case, then plz ignore the above comment.
If the logic used in archival is generic enough, maybe it is a good idea to move it to common

@longquanzheng longquanzheng changed the title Remove Deprecated code of Eventsv1 Remove Deprecated code of Events V1 Oct 11, 2019
@longquanzheng longquanzheng merged commit 58879ef into uber:master Oct 11, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 66.762% when pulling 7a4cdd1 on longquanzheng:eventsV1 into dda75d0 on uber:master.

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

Successfully merging this pull request may close these issues.

4 participants