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

feat(tiering): Simple OpManager #2781

Merged
merged 9 commits into from
Apr 4, 2024

Conversation

dranikpg
Copy link
Contributor

@dranikpg dranikpg commented Mar 27, 2024

OpManager is a higher level abstraction over DiskStorage that provides:

  • Tracking reads and comibing reads for the same offset
  • Tracking stashes and cancelling them
  • Enqueuing deletes after reads
  • Running the resolve loop after reading an entry

}

Future<std::string> OpManager::Read(const CompactObj& co) {
auto [offset, length] = co.GetExternalSlice();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this is still a draft. I just want to make sure I understand correctly, you current implementation does not takes into account that we want to group multiple small objects into a single page blob right?
f.e in here we will need to check which page this offset belongs page = offset & ~4095ULL
and then in the actions we will need to fill copy the string from the read result in offset - page with the relevant length right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, currently it doesn't, I will insert it right here in the future

@dranikpg
Copy link
Contributor Author

Actually I might need to redesign it a little so it fits with small key accumulators

@dranikpg
Copy link
Contributor Author

The plan is as follows:

the components managing small keys and gluing together pages will also use the OpManager for safe IO. Because they don't have any direct keys, they will refer to their operations with numeric identifiers.

Please review the design, I'll add a simple test in the meantime.

@dranikpg dranikpg changed the title WIP feat(tiering): Simple OpManager feat(tiering): Simple OpManager Mar 29, 2024
@dranikpg dranikpg marked this pull request as ready for review March 29, 2024 20:24
Copy link
Collaborator

@romange romange left a comment

Choose a reason for hiding this comment

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

btw, please make sure the whole build works on MacOS and linux specific parts are ignored on macos. See what I did with current tiered code

@romange
Copy link
Collaborator

romange commented Mar 31, 2024

please rebase as well.

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>
@dranikpg
Copy link
Contributor Author

Rebased, added one more test for delete after reads

@dranikpg dranikpg requested a review from romange March 31, 2024 20:46
// Small keys never begin at 0 because their keys are stored before values.
if (segment.offset % 4_KB != 0) {
DCHECK_LT(segment.length, 4_KB);
segment = {segment.offset / 4_KB * 4_KB, 4_KB};
Copy link
Collaborator

Choose a reason for hiding this comment

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

you loose the value offset, so how do you read the value from the page?
seems like a bug that requires a test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Read() I pass it twice

PrepareRead(segment).ForId(id, segment)

and the first modified segment isn't stored anywhere

ProcessRead will calculate it correctly

Copy link
Collaborator

Choose a reason for hiding this comment

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

wow, it's confusing by reading the code in the function.
suggestion: call the argument aligned_segment, dcheck on it and pass the aligned segment in Read.

Another thing - it will fail for large lengths like 4095 because you never align length. Unless you also handle it somewhere 😆

Copy link
Contributor Author

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

Added a test for the case you pointed out - no bugs found 🙂

// Small keys never begin at 0 because their keys are stored before values.
if (segment.offset % 4_KB != 0) {
DCHECK_LT(segment.length, 4_KB);
segment = {segment.offset / 4_KB * 4_KB, 4_KB};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Read() I pass it twice

PrepareRead(segment).ForId(id, segment)

and the first modified segment isn't stored anywhere

ProcessRead will calculate it correctly

src/server/tiering/op_manager.cc Show resolved Hide resolved
@dranikpg dranikpg requested a review from romange April 1, 2024 10:30
@romange
Copy link
Collaborator

romange commented Apr 3, 2024

I gave you comments that are not addressed yet

romange
romange previously approved these changes Apr 3, 2024
@dranikpg dranikpg requested a review from romange April 4, 2024 07:01
@dranikpg dranikpg merged commit 845f01c into dragonflydb:main Apr 4, 2024
7 checks passed
szinn pushed a commit to szinn/k8s-homelab that referenced this pull request Apr 16, 2024
…nfly ( v1.16.1 → v1.17.0 ) (#3473)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
|
[docker.dragonflydb.io/dragonflydb/dragonfly](https://togithub.com/dragonflydb/dragonfly)
| minor | `v1.16.1` -> `v1.17.0` |

---

### Release Notes

<details>
<summary>dragonflydb/dragonfly
(docker.dragonflydb.io/dragonflydb/dragonfly)</summary>

###
[`v1.17.0`](https://togithub.com/dragonflydb/dragonfly/releases/tag/v1.17.0)

[Compare
Source](https://togithub.com/dragonflydb/dragonfly/compare/v1.16.1...v1.17.0)

##### Dragonfly v1.17.0

Some prominent changes include:

- Improved performance for MGET operations
([#&#8203;2453](https://togithub.com/dragonflydb/dragonfly/issues/2453))
- Fix argument parsing in json.objkeys
([#&#8203;2872](https://togithub.com/dragonflydb/dragonfly/issues/2872))
- Fix ipv6 support for replication
([#&#8203;2889](https://togithub.com/dragonflydb/dragonfly/issues/2889))
- Support serialisation of bloom filters - saving to and loading from
snapshots
([#&#8203;2846](https://togithub.com/dragonflydb/dragonfly/issues/2846))
- Support of HLL PFADD
([#&#8203;2761](https://togithub.com/dragonflydb/dragonfly/issues/2761))
- Support bullmq workloads that do not have `{}` hashtags in their queue
names
([#&#8203;2890](https://togithub.com/dragonflydb/dragonfly/issues/2890))

##### What's Changed

- fix:
[#&#8203;2745](https://togithub.com/dragonflydb/dragonfly/issues/2745)
don't start migration process again after apply the same the same config
is applied by [@&#8203;BorysTheDev](https://togithub.com/BorysTheDev) in
[dragonflydb/dragonfly#2822
- feat(transaction): Idempotent callbacks (immediate runs) by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2453
- refactor(cluster): replace sync_id with node_id for slot migration
[#&#8203;2835](https://togithub.com/dragonflydb/dragonfly/issues/2835)
by [@&#8203;BorysTheDev](https://togithub.com/BorysTheDev) in
[dragonflydb/dragonfly#2838
- feat(tiering): Simple OpManager by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2781
- chore: implement path mutation for JsonFlat by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2805
- feat(cluster): add migration removing by config
[#&#8203;2835](https://togithub.com/dragonflydb/dragonfly/issues/2835)
by [@&#8203;BorysTheDev](https://togithub.com/BorysTheDev) in
[dragonflydb/dragonfly#2844
- chore: expose direct API on Bloom objects by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2845
- chore: generalize CompactObject::AllocateMR by
[@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2847
- feat(tiering): Simplest small bins by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2810
- refactor: clean cluster slot migration code by
[@&#8203;BorysTheDev](https://togithub.com/BorysTheDev) in
[dragonflydb/dragonfly#2848
- fix(tests): Fix numsub test by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2852
- fix: healthcheck for docker containers by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2853
- fix: possible crash in tls code by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2854
- fix(server): Do not block admin-port commands by
[@&#8203;chakaz](https://togithub.com/chakaz) in
[dragonflydb/dragonfly#2842
- fix(pytest): make pytests fail if server crash on shutdown by
[@&#8203;adiholden](https://togithub.com/adiholden) in
[dragonflydb/dragonfly#2827
- feat(server): add prints on takeover timeout by
[@&#8203;adiholden](https://togithub.com/adiholden) in
[dragonflydb/dragonfly#2856
- fix(pytest): dont check process return code on kill by
[@&#8203;adiholden](https://togithub.com/adiholden) in
[dragonflydb/dragonfly#2862
- fix: authorize the http connection to call commands by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2863
- feat(cluster): Send number of keys for incoming and outgoing
migrations. by [@&#8203;chakaz](https://togithub.com/chakaz) in
[dragonflydb/dragonfly#2858
- feat(tiering): TieredStorageV2 by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2849
- bug(server): set connection flags block/pause flag on all blocking
commands by [@&#8203;adiholden](https://togithub.com/adiholden) in
[dragonflydb/dragonfly#2816
- chore: serialize SBF by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2846
- fix: test_replicaof_reject_on_load crash on stop by
[@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2818
- feat(dbslice): Add self-laundering iterator in `DbSlice` by
[@&#8203;chakaz](https://togithub.com/chakaz) in
[dragonflydb/dragonfly#2815
- chore: License update by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2767
- fix(acl): incompatibilities with acl load by
[@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2867
- fix(json): make path optional in json.objkeys by
[@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2872
- fix: return wrong type errors for SET...GET command by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2874
- fix(redis replication): remove partial sync flow ,not supported yet by
[@&#8203;adiholden](https://togithub.com/adiholden) in
[dragonflydb/dragonfly#2865
- chore: limit traffic logger only to the main interface by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2877
- chore: relax repltakeover constraints to only exclude write commands
by [@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2873
- chore(replayer): Roll back to go1.18 by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2881
- fix: brpoplpush single shard to wake up blocked transactions by
[@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2875
- chore: LockTable tracks fingerprints of keys by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2839
- chore: reject TLS handshake when our listener is plain TCP by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2882
- Add support for Sparse HLL PFADD by
[@&#8203;azuredream](https://togithub.com/azuredream) in
[dragonflydb/dragonfly#2761
- feat server: bring visibility to script errors by
[@&#8203;adiholden](https://togithub.com/adiholden) in
[dragonflydb/dragonfly#2879
- chore: clean up REPLTAKEOVER flow by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2887
- chore(tiering): Move files and move kb literal to common by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2868
- chore(interpreter): Support object replies by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2885
- fix(ci/helm): Stick to v0.73.0 version of prom operator by
[@&#8203;Pothulapati](https://togithub.com/Pothulapati) in
[dragonflydb/dragonfly#2893
- fix(acl): authentication with UDS socket by
[@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2895
- feat(cluster): add repeated ACK if an error is happened by
[@&#8203;BorysTheDev](https://togithub.com/BorysTheDev) in
[dragonflydb/dragonfly#2892
- chore(blocking): Remove faulty DCHECK by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2898
- chore: add a clear link on how to build dragonfly from source by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2884
- feat(server): Allow configuration of hashtag extraction by
[@&#8203;chakaz](https://togithub.com/chakaz) in
[dragonflydb/dragonfly#2890
- fix: fix build under macos by
[@&#8203;BorysTheDev](https://togithub.com/BorysTheDev) in
[dragonflydb/dragonfly#2901
- fix(cluster_replication): replicate redis cluster node bug fix by
[@&#8203;adiholden](https://togithub.com/adiholden) in
[dragonflydb/dragonfly#2876
- fix(acl): skip http and add check on connection traversals by
[@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2883
- fix(zset): Better memory consumption calculation by
[@&#8203;chakaz](https://togithub.com/chakaz) in
[dragonflydb/dragonfly#2900
- fix: fix ld for num converting by
[@&#8203;BorysTheDev](https://togithub.com/BorysTheDev) in
[dragonflydb/dragonfly#2902
- chore: add help string for memory_fiberstack_vms_bytes by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2903
- fix(sanitizers): false positive fail on multi_test::Eval by
[@&#8203;kostasrim](https://togithub.com/kostasrim) in
[dragonflydb/dragonfly#2896
- chore: pull helio and add ipv6 replication test by
[@&#8203;dranikpg](https://togithub.com/dranikpg) in
[dragonflydb/dragonfly#2889
- chore: add ipv6 support for native linux release by
[@&#8203;romange](https://togithub.com/romange) in
[dragonflydb/dragonfly#2908

##### Huge thanks to all the contributors! ❤️

**Full Changelog**:
dragonflydb/dragonfly@v1.16.0...v1.17.0

</details>

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMDEuNSIsInVwZGF0ZWRJblZlciI6IjM3LjMwMS41IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZS9jb250YWluZXIiLCJ0eXBlL21pbm9yIl19-->

Co-authored-by: repo-jeeves[bot] <106431701+repo-jeeves[bot]@users.noreply.github.com>
@dranikpg dranikpg deleted the tiering-op-manager branch April 18, 2024 11:42
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.

None yet

3 participants