fix: Huge entries fail to load outside RDB / replication #4154
Conversation
We have an internal utility tool that we use to deserialize values in some use cases: * `RESTORE` * Cluster slot migration * `RENAME`, if the source and target shards are different We [recently](#3760) changed this area of the code, which caused this regression as it only handled RDB / replication streams. Fixes #4143
|
We will have to release a patch release for that. I would also want to add all the INFO improvements for 1.25.2 |
tests/dragonfly/cluster_test.py
Outdated
|
|
||
| await push_config(json.dumps(generate_config(nodes)), [node.admin_client for node in nodes]) | ||
|
|
||
| logging.debug(f"Generating huge {type}") |
There was a problem hiding this comment.
check out test_big_containers how it uses StaticSeeder
if we can do the same here and use the capture to compare
also this will be one test case and not running for each data type
tests/dragonfly/cluster_test.py
Outdated
| @pytest.mark.parametrize("type", ["list", "hash", "string", "set", "zset"]) | ||
| @dfly_args({"proactor_threads": 2, "cluster_mode": "yes"}) | ||
| @pytest.mark.asyncio | ||
| async def test_cluster_migration_huge_list(df_factory: DflyInstanceFactory, type): |
There was a problem hiding this comment.
rename test_cluster_migration_huge_list -> test_cluster_migration_huge_container
I see you test string data type above in params but its not clear to me what is
debug populate 1 k 10000 RAND TYPE string ELEMENTS 1000
what is the affect of elements param here?
There was a problem hiding this comment.
also lets add streams to tested types
There was a problem hiding this comment.
what is the affect of elements param here?
It doesn't have any effect, I thought it's a "free test" so why not, but I removed it
There was a problem hiding this comment.
also lets add streams to tested types
I don't have support for breaking streams, nor does StaticSeeder nor debug populate support streams :|
tests/dragonfly/generic_test.py
Outdated
| @@ -168,3 +169,41 @@ async def test_denyoom_commands(df_factory): | |||
|
|
|||
| # mget should not be rejected | |||
| await client.execute_command("mget x") | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("type", ["list", "hash", "string", "set", "zset"]) | |||
There was a problem hiding this comment.
we can do this test in unit test
see TEST_F(RdbTest, LoadHugeSet) for example
btw we can check the dump and restore command directly
There was a problem hiding this comment.
It's cleaner and with the seeder (your suggestion) also easier to do in pytests :)
| config.streamed = true; | ||
| } | ||
|
|
||
| if (auto ec = FromOpaque(*opaque_res, config, &pv); ec) { |
There was a problem hiding this comment.
can we remove (std::error_code RdbLoaderBase::FromOpaque(const OpaqueObj& opaque, CompactObj* pv)) function?
tests/dragonfly/cluster_test.py
Outdated
| collection_size=10_000, | ||
| variance=1, | ||
| samples=1, | ||
| types=[type], |
There was a problem hiding this comment.
I suggest instead of running a test case each data type you can pass all the types here
this will be a little faster ci and regression
* fix: Huge entries fail to load outside RDB / replication We have an internal utility tool that we use to deserialize values in some use cases: * `RESTORE` * Cluster slot migration * `RENAME`, if the source and target shards are different We [recently](#3760) changed this area of the code, which caused this regression as it only handled RDB / replication streams. Fixes #4143
We have an internal utility tool that we use to deserialize values in some use cases:
RESTORERENAME, if the source and target shards are differentWe recently changed this area of the code, which caused this regression as it only handled RDB / replication streams.
Fixes #4143