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

Versioning data merge algo and hybrid logical clock utils #4205

Merged
merged 5 commits into from
Apr 25, 2023

Conversation

bergundy
Copy link
Member

Another PR for version set / user data replication.

@bergundy bergundy requested a review from a team as a code owner April 21, 2023 23:17
@bergundy bergundy force-pushed the versioning-data-merge branch from 105151e to b565b4a Compare April 21, 2023 23:45
commonclock "go.temporal.io/server/common/clock"
)

var ErrClocksEqual = errors.New("HybridLogicalClocks are equal")
Copy link
Member

Choose a reason for hiding this comment

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

Is this used?

Comment on lines 51 to 52
// Merge and sort two sets of set IDs
func setContainsSetIDs(set *persistencepb.CompatibleVersionSet, ids []string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Wrong docstring

Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

This all makes sense to me

@bergundy bergundy merged commit b7c1b6b into temporalio:worker-versioning Apr 25, 2023
@bergundy bergundy deleted the versioning-data-merge branch April 25, 2023 03:21
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

package hybrid_logical_clock
Copy link
Member

Choose a reason for hiding this comment

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

does this really deserve a new package? just put it in common/clock.. they're used together anyway so anyone importing this needs to import that too

@@ -150,6 +153,7 @@ func NewEngine(
namespaceRegistry: namespaceRegistry,
keyResolver: resolver,
clusterMeta: clusterMeta,
timeSource: clock.NewRealTimeSource(), // No need to mock this at the moment
Copy link
Member

Choose a reason for hiding this comment

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

can we get this from fx instead? just for consistency. I can do it in another PR if you don't want to touch the fx stuff

dnr pushed a commit that referenced this pull request May 26, 2023
Note: This commit came from a feature branch and is not expected to build.
dnr pushed a commit to dnr/temporal that referenced this pull request May 26, 2023
…#4205)

Note: This commit came from a feature branch and is not expected to build.
dnr pushed a commit that referenced this pull request May 26, 2023
Note: This commit came from a feature branch and is not expected to build.
dnr pushed a commit that referenced this pull request May 26, 2023
Note: This commit came from a feature branch and is not expected to build.
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.

3 participants