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

Feature: add state machine range and subscribe API #4608

Merged
merged 15 commits into from
Apr 1, 2022
Merged

Feature: add state machine range and subscribe API #4608

merged 15 commits into from
Apr 1, 2022

Conversation

lichuang
Copy link
Contributor

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Feature: add state machine range and subscribe API

Changelog

  • New Feature

Related Issues

Fixes #issue #4607

Test Plan

Unit Tests

Stateless Tests

@vercel
Copy link

vercel bot commented Mar 28, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/FWxLPa2NLSSmjcMZtaDhZctyyjSG
✅ Preview: https://databend-git-fork-lichuang-addsmrangesubscribeapi-databend.vercel.app

[Deployment for 281b5b2 canceled]

@mergify
Copy link
Contributor

mergify bot commented Mar 28, 2022

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label Mar 28, 2022
@drmingdrmer
Copy link
Member

Why do we need range APIs for meta datas?

@lichuang
Copy link
Contributor Author

Why do we need range APIs for meta datas?

in etcd watch API, see https://etcd.io/docs/v3.2/learning/api/#watch-api, they have the key range:

message WatchCreateRequest { bytes key = 1; bytes range_end = 2; // ... }

@drmingdrmer
Copy link
Member

in etcd watch API, see https://etcd.io/docs/v3.2/learning/api/#watch-api, they have the key range:

message WatchCreateRequest { bytes key = 1; bytes range_end = 2; // ... }

On the doc page: Key, Range_End - The key range to watch..

If several given events happened as the following:

z:       z=3    
y:                     y=2
x: x=1         x=0
---+-----+-----+-------+------------------------------> time(revision)
   t1    t2    t3      t4

A watch request for key range [y,z], from t3, should only return event y=2 at t4.
In another word, to collect response data for watch, metasrv iterates all events in time order, then filters them by key range.
It does not need to iterate over the key range.

@lichuang
Copy link
Contributor Author

in etcd watch API, see https://etcd.io/docs/v3.2/learning/api/#watch-api, they have the key range:
message WatchCreateRequest { bytes key = 1; bytes range_end = 2; // ... }

On the doc page: Key, Range_End - The key range to watch..

If several given events happened as the following:

z:       z=3    
y:                     y=2
x: x=1         x=0
---+-----+-----+-------+------------------------------> time(revision)
   t1    t2    t3      t4

A watch request for key range [y,z], from t3, should only return event y=2 at t4. In another word, to collect response data for watch, metasrv iterates all events in time order, then filters them by key range. It does not need to iterate over the key range.

I wonder if or not return the current event at the first time when a watcher has created, but in etcd doc and code, seems they do not return event when create watcher.ok, i delete the range API.

common/meta/raft-store/src/state_machine/sm.rs Outdated Show resolved Hide resolved
common/meta/raft-store/src/state_machine/sm.rs Outdated Show resolved Hide resolved
common/meta/raft-store/src/state_machine/sm.rs Outdated Show resolved Hide resolved
common/meta/raft-store/src/state_machine/sm.rs Outdated Show resolved Hide resolved
common/meta/raft-store/src/state_machine/sm.rs Outdated Show resolved Hide resolved
Feature: add state machine range and subscribe API

Feature: remove unused comment
@drmingdrmer
Copy link
Member

@lichuang
Is this ready to review or are you gonna cleanup the commits?

1 similar comment
@drmingdrmer
Copy link
Member

@lichuang
Is this ready to review or are you gonna cleanup the commits?

@lichuang
Copy link
Contributor Author

Is this ready to review or are you gonna cleanup the commits?

ready

@drmingdrmer
Copy link
Member

I wonder if or not return the current event at the first time when a watcher has created, but in etcd doc and code, seems they do not return event when create watcher.ok, i delete the range API.

The range APIs are still there.

BTW, another issue is that range-API is not snapshot-based. The result may be inconsistent.

@lichuang
Copy link
Contributor Author

I wonder if or not return the current event at the first time when a watcher has created, but in etcd doc and code, seems they do not return event when create watcher.ok, i delete the range API.

The range APIs are still there.

BTW, another issue is that range-API is not snapshot-based. The result may be inconsistent.

sad, when rebase source it does not remove the range API...

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

LGTM

@lichuang lichuang requested a review from BohuTANG April 1, 2022 01:46
@BohuTANG BohuTANG merged commit 0278dfd into databendlabs:main Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-review pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants