chore: reduce includes #6464

dranikpg · 2026-01-24T11:39:42Z

Weekend cleanup 🧹

Make server/journal its own target
Reduce facade/facade_types to move stats structs into facade/facade_stats.h
Reduce includes in common.h and move GenRandomHex to separate header/cc
Fix long chain of absl/other includes that is needed with smaller common.h

dranikpg · 2026-01-24T13:24:14Z

augment review

augmentcode · 2026-01-24T13:28:57Z

🤖 Augment PR Summary

Summary: Refactors headers/targets to reduce transitive includes and tighten compilation boundaries.

Changes:

Extracts src/server/journal into a dedicated dfly_journal library target and wires it into dfly_transaction; relocates the journal test registration.
Moves facade statistics types (ConnectionStats, ReplyStats, FacadeStats, and tl_facade_stats) out of facade_types.h into a new facade/facade_stats.h, updating includes across facade/server.
Reduces dependencies in server/common.h via forward declarations and an out-of-line ScanOpts destructor to avoid including GlobMatcher in the header.
Moves GetRandomHex into core/detail/gen_utils.h and updates call sites (server code + tests) to use the new location.
Adjusts miscellaneous includes (Abseil, TLS helpers, cluster family, etc.) to match the slimmer headers.
Switches ScriptMgr::Insert to return nonstd::expected explicitly (equivalent to io::Result).

Technical Notes: The PR is primarily a build hygiene/compile-time cleanup; the functional behavior should remain unchanged aside from the build target reshuffling.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 1 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-01-24T13:28:58Z

src/server/CMakeLists.txt

            serializer_commons.cc
            acl/acl_log.cc slowlog.cc channel_store.cc)
-cxx_link(dfly_transaction dfly_core strings_lib TRDP::fast_float TRDP::hdr_histogram)
+cxx_link(dfly_transaction dfly_core dfly_journal strings_lib TRDP::fast_float TRDP::hdr_histogram)


dfly_transaction now links dfly_journal, while dfly_journal links back to dfly_transaction (and also links dragonfly_lib, which already links dfly_transaction), creating a circular static-library dependency that can lead to unresolved symbols depending on link order (since cxx_link() is a plain target_link_libraries). Consider verifying this in all build configurations/toolchains.

Other Locations

src/server/journal/CMakeLists.txt:5

_{🤖 Was this useful? React with 👍 or 👎}

Signed-off-by: Vladislav Oleshko <[email protected]>

augmentcode

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

Copilot

Pull request overview

This PR performs a significant refactoring to reduce header dependencies and improve build modularity by:

Changes:

Created dfly_journal as a separate build target instead of including source files directly
Extracted stats structures from facade/facade_types.h into a new facade/facade_stats.h header
Moved GetRandomHex utility function from server/common.h to core/detail/gen_utils.h
Reduced includes in server/common.h by using forward declarations for GlobMatcher and CompactObjType
Fixed include chains across numerous files to resolve dependencies broken by the smaller common.h
Changed script_mgr to use nonstd::expected instead of io::Result to avoid unnecessary io.h include

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/server/journal/CMakeLists.txt	Converts journal from included sources to standalone library target
src/server/CMakeLists.txt	Updates to link dfly_transaction against dfly_journal library
src/facade/facade_stats.h	New header extracting ConnectionStats, ReplyStats, and FacadeStats from facade_types.h
src/facade/facade_types.h	Removes stats structures now in facade_stats.h
src/core/detail/gen_utils.h	New header-only file containing GetRandomHex utility
src/core/CMakeLists.txt	Minor formatting changes to source list
src/server/common.h	Reduces includes, adds forward declarations, moves GetRandomHex
src/server/common.cc	Implements ScanOpts destructor needed for forward declarations
src/server/server_state.h	Moves inline methods to .cc file, adds forward declaration for ConnectionStats
src/server/server_state.cc	Implements methods previously inline in header
src/server/script_mgr.h	Changes return type from io::Result to nonstd::expected
src/server/table.h	Replaces histogram include with forward declaration
src/server/stream_family.h	Simplifies to use CompactValue forward declaration
src/server/rdb_save.h	Adds RdbTypeFreqMap definition moved from common.h
src/server/*_family.cc	Adds absl/strings/ascii.h includes as needed
src/server/*/test files	Adds core/detail/gen_utils.h and updates GetRandomHex calls
src/facade/*.cc	Adds facade_stats.h includes as needed

Copilot · 2026-01-26T08:04:21Z

src/server/server_state.cc

  channel_store_ = replacement;
 }

+void ServerState::RecordCmd(const bool is_main_conn) {


The parameter is declared as bool is_main_conn in the header file but implemented as const bool is_main_conn here. While legal, this inconsistency should be avoided. The const qualifier on value parameters in function definitions is typically omitted as it's not part of the function signature and only affects the implementation.

Copilot · 2026-01-26T08:04:22Z

src/server/journal/CMakeLists.txt

+    cmd_serializer.cc tx_executor.cc
+    journal.cc types.cc journal_slice.cc
+    serializer.cc executor.cc streamer.cc)
+target_link_libraries(dfly_journal dfly_transaction dragonfly_lib dfly_facade base io)


Circular dependency detected: dfly_journal links against dfly_transaction (line 5), but dfly_transaction also links against dfly_journal (line 38 in src/server/CMakeLists.txt). This will cause build issues. The dependency should only go in one direction - either journal depends on transaction, or transaction depends on journal, but not both. Based on typical layering, journal should likely depend on transaction (not the other way around).

Suggested change

target_link_libraries(dfly_journal dfly_transaction dragonfly_lib dfly_facade base io)

target_link_libraries(dfly_journal dragonfly_lib dfly_facade base io)

Sadly everyhing depends on everything

IMHO we should take care of that... I opened an issue: #6479

Also my concern here is that we add a new circular dependency:

before: journal sources are part of dfly_transaction that links with dfly_lib -> so no circular deps
now: separate lib (dflyjournal) that links both with dfly_transaction but and vice versa -> new circular dep

This change is suspicious unless I am missing something with the linking order.

Copilot · 2026-01-26T08:04:22Z

src/facade/facade_stats.h

@@ -0,0 +1,100 @@
+#pragma once


Missing copyright header. New files should include the standard DragonflyDB copyright header at the top of the file, similar to other files in the codebase.

dranikpg · 2026-01-26T09:25:13Z

src/facade/facade_stats.h

+
+namespace facade {
+
+struct ConnectionStats {


all of this is moved from facade_types.h

dranikpg · 2026-01-26T09:26:05Z

src/server/journal/CMakeLists.txt

+    cmd_serializer.cc tx_executor.cc
+    journal.cc types.cc journal_slice.cc
+    serializer.cc executor.cc streamer.cc)
+target_link_libraries(dfly_journal dfly_transaction dragonfly_lib dfly_facade base io)


Sadly everyhing depends on everything

BorysTheDev

Great job

BorysTheDev · 2026-01-27T09:13:33Z

Honestly, I hate common.h it's just a trash bin

kostasrim · 2026-01-27T07:55:44Z

src/facade/facade_stats.h

+  uint64_t tls_accept_disconnects = 0;  // number of TLS socket disconnects during the handshake
+                                        //


Suggested change

uint64_t tls_accept_disconnects = 0; // number of TLS socket disconnects during the handshake

//

// number of TLS socket disconnects during the handshake

uint64_t tls_accept_disconnects = 0;

kostasrim · 2026-01-27T07:56:43Z

src/facade/facade_stats.h

+    int64_t count = 0;
+    int64_t total_duration = 0;
+
+    SendStats& operator+=(const SendStats& other) {


Not applicable here but n general, we also have the spaceship operator now :)

kostasrim · 2026-01-27T07:57:17Z

src/facade/resp_validator.cc


 #include <cstdint>
 #include <fstream>
+#include <iostream>


kostasrim · 2026-01-27T07:59:21Z

src/server/journal/CMakeLists.txt

+    cmd_serializer.cc tx_executor.cc
+    journal.cc types.cc journal_slice.cc
+    serializer.cc executor.cc streamer.cc)
+target_link_libraries(dfly_journal dfly_transaction dragonfly_lib dfly_facade base io)


IMHO we should take care of that... I opened an issue: #6479

kostasrim · 2026-01-27T12:03:53Z

src/server/journal/CMakeLists.txt

+    cmd_serializer.cc tx_executor.cc
+    journal.cc types.cc journal_slice.cc
+    serializer.cc executor.cc streamer.cc)
+target_link_libraries(dfly_journal dfly_transaction dragonfly_lib dfly_facade base io)


Also my concern here is that we add a new circular dependency:

before: journal sources are part of dfly_transaction that links with dfly_lib -> so no circular deps
now: separate lib (dflyjournal) that links both with dfly_transaction but and vice versa -> new circular dep

This change is suspicious unless I am missing something with the linking order.

dranikpg added 2 commits January 24, 2026 14:38

chore: reduce includes

1d269bd

some updates

94e8c29

augmentcode bot reviewed Jan 24, 2026

View reviewed changes

more fixes

e60149c

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg force-pushed the include-war-1 branch from 8ebe8c2 to e60149c Compare January 24, 2026 17:58

more fixes

eed663a

Signed-off-by: Vladislav Oleshko <[email protected]>

dranikpg force-pushed the include-war-1 branch from 242db78 to eed663a Compare January 25, 2026 07:59

dranikpg requested review from BorysTheDev and kostasrim January 26, 2026 07:58

dranikpg marked this pull request as ready for review January 26, 2026 07:58

Copilot AI review requested due to automatic review settings January 26, 2026 07:58

Copilot started reviewing on behalf of dranikpg January 26, 2026 07:59 View session

augmentcode bot reviewed Jan 26, 2026

View reviewed changes

Copilot AI reviewed Jan 26, 2026

View reviewed changes

small fixes

e293ecb

dranikpg commented Jan 26, 2026

View reviewed changes

BorysTheDev approved these changes Jan 27, 2026

View reviewed changes

kostasrim reviewed Jan 27, 2026

View reviewed changes

	target_link_libraries(dfly_journal dfly_transaction dragonfly_lib dfly_facade base io)
	target_link_libraries(dfly_journal dragonfly_lib dfly_facade base io)

		uint64_t tls_accept_disconnects = 0; // number of TLS socket disconnects during the handshake
		//


		namespace facade {

		struct ConnectionStats {

chore: reduce includes #6464

Are you sure you want to change the base?

chore: reduce includes #6464

Conversation

dranikpg commented Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dranikpg commented Jan 24, 2026

Uh oh!

augmentcode bot commented Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode bot Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BorysTheDev left a comment

Choose a reason for hiding this comment

Uh oh!

BorysTheDev commented Jan 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dranikpg commented Jan 24, 2026 •

edited

Loading

augmentcode bot commented Jan 24, 2026 •

edited

Loading