Skip to content

rocksdb feature flag#136

Merged
JoshOrndorff merged 3 commits intomainfrom
remove-rocks
Nov 9, 2023
Merged

rocksdb feature flag#136
JoshOrndorff merged 3 commits intomainfrom
remove-rocks

Conversation

@muraca
Copy link
Copy Markdown
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.

@muraca muraca requested a review from JoshOrndorff November 7, 2023 13:51
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
@JoshOrndorff
Copy link
Copy Markdown
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
Copy Markdown
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>
@github-actions
Copy link
Copy Markdown

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
Copy Markdown
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
@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