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

rocksdb feature flag #136

Merged
merged 3 commits into from
Nov 9, 2023
Merged

rocksdb feature flag #136

merged 3 commits into from
Nov 9, 2023

Conversation

muraca
Copy link
Collaborator

@muraca muraca commented Nov 7, 2023

Use ParityDB as the default database, and allow RocksDB to be used by compiling the node with the feature flag rocksdb.
This was done because building RocksDB is only a waste of time and resources if used for CI and tests.

Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
@JoshOrndorff
Copy link
Contributor

My first thoughts were, "Is this really necessary, is parity db good enough, does Tuxedo need to be opinionated about the DB, etc".

But the compile time is a pretty compelling reason. How many hours have I wasted and CO2 emitted compiling lib-rocks-db-sys.

So in the end I actually do support this change, I just request that you document this a little bit better because this is a template and we don't want users getting confused and frustrated by things that are not so core to Tuxedo.

So for example, please provide a better PR description. And maybe also a comment in the Cargo.toml file where we disable the default features.

FYI, if the rock-db feature is enabled, the default DB is rocks DB. And it is a default feature. So in that sense, the default in the ecosystem is still to use Rocks DB.
https://github.com/paritytech/polkadot-sdk/blob/d347d68868a38841ba51f2e91cef945a7ea50dd4/substrate/client/cli/src/config.rs#L454-L463

@muraca
Copy link
Collaborator Author

muraca commented Nov 9, 2023

FYI, if the rock-db feature is enabled, the default DB is rocks DB. And it is a default feature. So in that sense, the default in the ecosystem is still to use Rocks DB.

Shall I use the same pattern? I thought it was an overkill, but if it's still the standard for Substrate then maybe it's the right thing to do.

Edit: I will do it this way. It's a clean and elegant solution.

@muraca muraca changed the title remove rocksdb from dependencies rocksdb feature flag Nov 9, 2023
Signed-off-by: muraca <mmuraca247@gmail.com>
Copy link

github-actions bot commented Nov 9, 2023

Coverage after merging remove-rocks into main will be

64.52%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
tuxedo-core/aggregator/src
   lib.rs99.29%100%100%99.26%117, 21
tuxedo-core/no_bound/src
   clone_no_bound.rs36.63%100%30%37.36%24, 32–39, 56, 59–72, 74–87, 89, 91–95, 98–99
   debug_no_bound.rs32.73%100%30%33%101–105, 108–109, 24, 32–42, 58, 60–78, 80–95, 97–99
   default_no_bound.rs19.11%100%16.67%19.31%100–122, 124–131, 133, 136–147, 151–157, 32–39, 51, 54–80, 83–88, 91–99
   lib.rs100%100%100%100%
tuxedo-core/src
   constraint_checker.rs50%100%47.37%50.75%110, 71–91, 93–95
   dynamic_typing.rs84.03%100%72.73%88.37%57
   executive.rs91.52%100%92.68%91.38%116, 142, 175, 207, 220–228, 236, 247, 298, 325, 376–381, 383, 393–394, 399–400, 403, 405–406, 409–410, 412, 416–437, 439, 443–444, 446–447, 449, 452–469, 54
   genesis.rs30.43%100%33.33%30.30%114, 36–49, 60, 62–65, 67–68, 70–72, 75–79, 81–83, 85–97
   inherents.rs7.41%100%12.50%6.52%120–122, 158–159, 164–178, 180–203, 212–217, 219–228, 230, 56–58, 63–68, 70–78, 81, 83
   types.rs70.21%100%54.35%75.35%13, 143, 36, 86, 88–91, 93–99
   utxo_set.rs90.91%100%100%88.89%39–40
   verifier.rs87.16%100%71.60%91.05%120, 154, 28, 54–55, 68
tuxedo-template-runtime/src
   lib.rs33.64%100%25%35.02%100–102, 104, 208, 211, 216–218, 222–224, 237, 239, 241, 243, 245, 247, 249, 251, 254, 256, 261–262, 270–291, 294–321, 324–435, 61–66, 97–99
wallet/src
   amoeba.rs0%100%0%0%100–106, 109–110, 112–118, 120–127, 19–48, 51–52, 54–99
   cli.rs0%100%0%0%103, 108, 119, 125, 14, 17, 19, 28, 33, 38, 45, 56, 68, 91
   keystore.rs0%100%0%0%31–33, 38–45, 47–48, 51, 53–59, 65–73, 76–78, 80–82, 85–91, 93–94
   main.rs0%100%0%0%101–102, 105–117, 120, 122–126, 129–133, 135–144, 146–147, 151–161, 164–165, 167, 170–171, 173, 175, 177–180, 182–184, 186, 190–197, 200–203, 205–207, 210–212, 214, 216, 219–225, 228–241, 244–247, 249–259, 26, 260, 262, 27–30, 33, 36–38, 40–41, 44, 46, 48–49, 53, 56–62, 65, 67–69, 73–76, 78, 80, 82–83, 86–87, 89, 91–93, 98–99
   money.rs0%100%0%0%100–101, 105, 109–112, 114, 119–125, 127–131, 134–135, 139–144, 146–147, 22–28, 31–49, 53–59, 65–72, 74, 78–83, 87, 90, 92, 95–98
   output_filter.rs100%100%100%100%
   rpc.rs0%100%0%0%16–21, 24–26, 28–44, 47–53, 55–58, 60–61
   sync.rs0%100%0%0%103–106, 113–114, 116, 119–122, 127–129, 132–133, 136–141, 143–145, 148, 150–151, 156–159, 162–163, 170–174, 176–182, 185–187, 189–190, 193–194, 199–202, 205, 207–208, 213–216, 219, 221–222, 225–231, 233–234, 237–238, 241–242, 245–246, 250–256, 259–263, 266–268, 271–279, 281, 285, 287–288, 291–292, 295–302, 304–305, 308–309, 311, 313–314, 318–320, 322–323, 325–326, 328–329, 332–334, 336–337, 339–340, 342–343, 347, 349–350, 354, 356–361, 364–365, 368–370, 373, 376–379, 381, 384–387, 390, 393–394, 397–398, 403–408, 410, 412, 417–420, 423–424, 427–432, 434, 437–438, 442–443, 445, 447–449, 451–454, 457–458, 46–50, 54, 57–58, 61, 63–77, 82–86, 88–89, 94–99
wardrobe/amoeba/src
   lib.rs58.17%100%26.83%69.64%121, 139–140, 178–179, 81–82
   tests.rs100%100%100%100%
wardrobe/kitties/src
   lib.rs54.40%100%33.05%63.81%100–101, 105–109, 115–117, 121–122, 125–127, 131–134, 138–140, 142–143, 147–151, 174–175, 178–185, 188, 191, 193, 195, 197, 199, 201, 203, 205, 207, 209, 211, 213, 215, 217, 219, 221, 223, 225, 354, 38, 386, 389, 39–40, 42–43, 449, 46–51, 54–56, 58–59, 64–68, 75–77, 79–80, 85–89, 96–98
   tests.rs99.61%100%98.25%99.78%
wardrobe/money/src
   lib.rs42%100%18.75%52.94%100, 103, 105, 107, 111, 114, 117, 175, 21–23, 32–34, 36–37, 40–46, 50, 59–61, 63–65, 68–71, 75–77, 86–87, 90–97
   tests.rs100%100%100%100%
wardrobe/poe/src
   lib.rs0%100%0%0%100, 108–117, 121–122, 128–129, 134–143, 147–151, 153–154, 167–168, 173–179, 39, 52, 55, 57, 59, 61, 65, 84–86, 91–99
wardrobe/runtime_upgrade/src
   lib.rs0%100%0%0%100–102,

Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

I also thought their way of determining the default was overkill. I wonder if it is for backwards compatibility for nodes that ahve been running since before parity db?

Anyway, I'm happy with the way you have it and look forward to the faster builds.

@JoshOrndorff JoshOrndorff merged commit 76bc8b1 into main Nov 9, 2023
6 checks passed
@JoshOrndorff JoshOrndorff deleted the remove-rocks branch November 9, 2023 16:44
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.

2 participants