Add ddst_dict_init api support for rocksdb dd #1297
Conversation
sunshine-Chun
force-pushed
the
fb-mysql-8.0.28-dd-api2
branch
from
66fa355 to
4fddbc6
Compare
storage/rocksdb/ha_rocksdb.cc
Outdated
|
|
||
| static Plugin_tablespace dd_space(rocksdb_dd_space_name, "", "", "", | ||
| rocksdb_hton_name); | ||
| tablespaces->push_back(&dd_space); |
There was a problem hiding this comment.
On a second thought, tablespaces are purely InnoDB concept. So nothing should be added here?
There was a problem hiding this comment.
I think DD expect mysql namespace always exists.. Let me find related code in a while..
There was a problem hiding this comment.
The current DD layer assumes it does, but I am patching it out in my prototype.
I am concerned about pretending something exists when MyRocks does not even support the concept of it.
sunshine-Chun
force-pushed
the
fb-mysql-8.0.28-dd-api2
branch
2 times, most recently
from
88b36f4 to
7897e70
Compare
storage/rocksdb/rdb_native_dd.h
Outdated
|
|
||
| namespace myrocks { | ||
|
|
||
| constexpr const char *rocksdb_dd_space_name = "mysql"; |
There was a problem hiding this comment.
| constexpr const char *rocksdb_dd_space_name = "mysql"; | |
| constexpr const char rocksdb_dd_space_name[] = "mysql"; |
The pointer version works, but its sizeof is 8, which is very annoying to find
|
Do we still need this diff. Sounds like we are not adding the. dd_space in the ddst_dict_init function now. |
|
As with the rest of the current diffs, it's hard to say, as the code is too incomplete to run. IMHO we can hold until it becomes clear. |
|
@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@laurynas-biveinis , according to previous conversation, we may still need this mysql tablespace. Currently #1319 will fail if there is testcase using rocksdb DD. |
sunshine-Chun
force-pushed
the
fb-mysql-8.0.28-dd-api2
branch
from
7897e70 to
8dcbca4
Compare
|
@sunshine-Chun has updated the pull request. You must reimport the pull request before landing. |
|
@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
sunshine-Chun
force-pushed
the
fb-mysql-8.0.28-dd-api2
branch
from
8dcbca4 to
4d98701
Compare
|
@sunshine-Chun has updated the pull request. You must reimport the pull request before landing. |
sunshine-Chun
force-pushed
the
fb-mysql-8.0.28-dd-api2
branch
from
4d98701 to
0593d65
Compare
|
@sunshine-Chun has updated the pull request. You must reimport the pull request before landing. |
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
sunshine-Chun
force-pushed
the
fb-mysql-8.0.28-dd-api2
branch
from
0593d65 to
0f1e9a8
Compare
|
@sunshine-Chun has updated the pull request. You must reimport the pull request before landing. |
|
@sunshine-Chun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@sunshine-Chun , @luqun, It is true that we'll need the mysql tablespace. But, in the current prototype it is InnoDB that sets it up, thus at this point I am not comfortable to committing to RocksDB creating it. We could quickly merge a PR for |
|
@sunshine-Chun , please close this PR |
4 participants
Summary: Add ddst_dict_init api support for rocksdb dd
Test Plan: Test locally, not crush....
Reviewers:
Subscribers:
Tasks:
Tags: