Refactor source file structure #989

PragmaTwice · 2022-10-14T09:22:46Z

Mostly follows #977 (comment), except:

log_collector and tls_util is moved to network/
add task_runner to common/

PragmaTwice · 2022-10-14T09:42:32Z

log_collector is not a network thing, and i think it should be put under common

I think both common and network is ok, I put it into network because I find that this file relies on redis_reply and is using redis protocol. I will move it to common.

caipengbo · 2022-10-14T09:44:01Z

@PragmaTwice log_collector is not a network-related thing, it is a statistical thing, how about adding a stats directory with log_collector.h and stats.h or even redis_disk.h (or can rename it disk_stats.h?)

PragmaTwice · 2022-10-14T09:44:39Z

@PragmaTwice log_collector is not a network-related thing, it is a statistical thing, how about adding a stats directory with log_collector.h and stats.h or even redis_disk.h (or can rename it disk_stats.h?)

Greate idea. I will do it.

caipengbo · 2022-10-14T09:54:23Z

geohash.h file appears to be used only by redis_geo.h. I don't think it's appropriate to put it in storage. Can we just put it inside types?

PragmaTwice · 2022-10-14T09:54:58Z

geohash.h file appears to be used only by redis_geo.h. I don't think it's appropriate to put it in storage. Can we just put it inside types?

Agree with that.

caipengbo · 2022-10-14T09:57:42Z

log_collector is not a network-related thing, it is a statistical thing, how about adding a stats directory with log_collector.h and stats.h or even redis_disk.h (or can rename it disk_stats.h?)

We renamed redis_disk.h to disk_stats.h, do you think that is reasonable? @tanruixiang

PragmaTwice · 2022-10-14T10:01:34Z

Current source structure:

$ tree src
├── cluster
│   ├── cluster.cc
│   ├── cluster.h
│   ├── redis_slot.cc
│   ├── redis_slot.h
│   ├── replication.cc
│   ├── replication.h
│   ├── slot_import.cc
│   ├── slot_import.h
│   ├── slot_migrate.cc
│   └── slot_migrate.h
├── commands
│   ├── redis_cmd.cc
│   └── redis_cmd.h
├── common
│   ├── cron.cc
│   ├── cron.h
│   ├── db_util.h
│   ├── encoding.cc
│   ├── encoding.h
│   ├── event_util.h
│   ├── fd_util.h
│   ├── parse_util.h
│   ├── rand.cc
│   ├── rand.h
│   ├── rocksdb_crc32c.h
│   ├── rw_lock.h
│   ├── scope_exit.h
│   ├── sha1.cc
│   ├── sha1.h
│   ├── status.h
│   ├── task_runner.cc
│   ├── task_runner.h
│   ├── util.cc
│   └── util.h
├── config
│   ├── config.cc
│   ├── config.h
│   ├── config_type.h
│   ├── config_util.cc
│   └── config_util.h
├── main.cc
├── network
│   ├── redis_connection.cc
│   ├── redis_connection.h
│   ├── redis_reply.cc
│   ├── redis_reply.h
│   ├── redis_request.cc
│   ├── redis_request.h
│   ├── server.cc
│   ├── server.h
│   ├── tls_util.cc
│   ├── tls_util.h
│   ├── worker.cc
│   └── worker.h
├── stats
│   ├── disk_stats.cc
│   ├── disk_stats.h
│   ├── log_collector.cc
│   ├── log_collector.h
│   ├── stats.cc
│   └── stats.h
├── storage
│   ├── batch_extractor.cc
│   ├── batch_extractor.h
│   ├── compact_filter.cc
│   ├── compact_filter.h
│   ├── compaction_checker.cc
│   ├── compaction_checker.h
│   ├── event_listener.cc
│   ├── event_listener.h
│   ├── lock_manager.cc
│   ├── lock_manager.h
│   ├── redis_db.cc
│   ├── redis_db.h
│   ├── redis_metadata.cc
│   ├── redis_metadata.h
│   ├── redis_pubsub.cc
│   ├── redis_pubsub.h
│   ├── scripting.cc
│   ├── scripting.h
│   ├── storage.cc
│   ├── storage.h
│   ├── table_properties_collector.cc
│   └── table_properties_collector.h
├── types
│   ├── geohash.cc
│   ├── geohash.h
│   ├── redis_bitmap.cc
│   ├── redis_bitmap.h
│   ├── redis_bitmap_string.cc
│   ├── redis_bitmap_string.h
│   ├── redis_geo.cc
│   ├── redis_geo.h
│   ├── redis_hash.cc
│   ├── redis_hash.h
│   ├── redis_list.cc
│   ├── redis_list.h
│   ├── redis_set.cc
│   ├── redis_set.h
│   ├── redis_sortedint.cc
│   ├── redis_sortedint.h
│   ├── redis_stream_base.cc
│   ├── redis_stream_base.h
│   ├── redis_stream.cc
│   ├── redis_stream.h
│   ├── redis_string.cc
│   ├── redis_string.h
│   ├── redis_zset.cc
│   └── redis_zset.h
├── valgrind.sup
└── version.h.in

8 directories, 104 files

tisonkun

Great work! Comments inline.

tisonkun · 2022-10-14T10:12:16Z

CMakeLists.txt

@@ -148,12 +148,12 @@ find_library(FOUND_UNWIND_LIB unwind)
 set(WARNING_FLAGS -Wall -Wpedantic -Wsign-compare -Wreturn-type)

 # kvrocks objects target
-file(GLOB KVROCKS_SRCS src/*.cc)
+file(GLOB_RECURSE KVROCKS_SRCS src/*.cc)


Does the build benefit from separating compile units? For example, one .o per directory and for better incremental compilation. If it does we can implement it here or as a follow-up.

I think there is not much dependency between translation units. In theory, if there are 1000 source files, a 1000-core cpu and enough memory, we can compile completely in parallel.

tisonkun · 2022-10-14T10:12:37Z

CMakeLists.txt

 list(FILTER KVROCKS_SRCS EXCLUDE REGEX src/main.cc)

 add_library(kvrocks_objs OBJECT ${KVROCKS_SRCS})

-target_include_directories(kvrocks_objs PUBLIC src ${PROJECT_BINARY_DIR})
+target_include_directories(kvrocks_objs PUBLIC src src/common ${PROJECT_BINARY_DIR})


What's the reason of this change?

Since header files in common is frequently used, so e.g. it can be just "status.h" rather than "common/status.h".

src/cluster/cluster.cc

tanruixiang · 2022-10-14T10:33:23Z

We renamed redis_disk.h to disk_stats.h, do you think that is reasonable? @tanruixiang

Yes, I think it's reasonable.

git-hulk · 2022-10-14T16:26:15Z

@PragmaTwice After looking detail, it seems server is better than network to replace service. It's my bad that didn't mention this option, others are good to me. I think we can merge it as soon as possible to avoid conflicts.

PragmaTwice · 2022-10-15T04:32:54Z

@PragmaTwice After looking detail, it seems server is better than network to replace service. It's my bad that didn't mention this option, others are good to me. I think we can merge it as soon as possible to avoid conflicts.

Done. @git-hulk

The current diff in github review page seems to be a mess, maybe because I rebase some commits.

PragmaTwice · 2022-10-15T07:01:28Z

The current diff in github review page seems to be a mess, maybe because I rebase some commits.

Solved by re-rebase and force-push.

Current source structure:

src
├── cluster
│   ├── cluster.cc
│   ├── cluster.h
│   ├── redis_slot.cc
│   ├── redis_slot.h
│   ├── replication.cc
│   ├── replication.h
│   ├── slot_import.cc
│   ├── slot_import.h
│   ├── slot_migrate.cc
│   └── slot_migrate.h
├── commands
│   ├── redis_cmd.cc
│   └── redis_cmd.h
├── common
│   ├── cron.cc
│   ├── cron.h
│   ├── db_util.h
│   ├── encoding.cc
│   ├── encoding.h
│   ├── event_util.h
│   ├── fd_util.h
│   ├── parse_util.h
│   ├── rand.cc
│   ├── rand.h
│   ├── rocksdb_crc32c.h
│   ├── rw_lock.h
│   ├── scope_exit.h
│   ├── sha1.cc
│   ├── sha1.h
│   ├── status.h
│   ├── task_runner.cc
│   ├── task_runner.h
│   ├── util.cc
│   └── util.h
├── config
│   ├── config.cc
│   ├── config.h
│   ├── config_type.h
│   ├── config_util.cc
│   └── config_util.h
├── main.cc
├── server
│   ├── redis_connection.cc
│   ├── redis_connection.h
│   ├── redis_reply.cc
│   ├── redis_reply.h
│   ├── redis_request.cc
│   ├── redis_request.h
│   ├── server.cc
│   ├── server.h
│   ├── tls_util.cc
│   ├── tls_util.h
│   ├── worker.cc
│   └── worker.h
├── stats
│   ├── disk_stats.cc
│   ├── disk_stats.h
│   ├── log_collector.cc
│   ├── log_collector.h
│   ├── stats.cc
│   └── stats.h
├── storage
│   ├── batch_extractor.cc
│   ├── batch_extractor.h
│   ├── compact_filter.cc
│   ├── compact_filter.h
│   ├── compaction_checker.cc
│   ├── compaction_checker.h
│   ├── event_listener.cc
│   ├── event_listener.h
│   ├── lock_manager.cc
│   ├── lock_manager.h
│   ├── redis_db.cc
│   ├── redis_db.h
│   ├── redis_metadata.cc
│   ├── redis_metadata.h
│   ├── redis_pubsub.cc
│   ├── redis_pubsub.h
│   ├── scripting.cc
│   ├── scripting.h
│   ├── storage.cc
│   ├── storage.h
│   ├── table_properties_collector.cc
│   └── table_properties_collector.h
├── types
│   ├── geohash.cc
│   ├── geohash.h
│   ├── redis_bitmap.cc
│   ├── redis_bitmap.h
│   ├── redis_bitmap_string.cc
│   ├── redis_bitmap_string.h
│   ├── redis_geo.cc
│   ├── redis_geo.h
│   ├── redis_hash.cc
│   ├── redis_hash.h
│   ├── redis_list.cc
│   ├── redis_list.h
│   ├── redis_set.cc
│   ├── redis_set.h
│   ├── redis_sortedint.cc
│   ├── redis_sortedint.h
│   ├── redis_stream_base.cc
│   ├── redis_stream_base.h
│   ├── redis_stream.cc
│   ├── redis_stream.h
│   ├── redis_string.cc
│   ├── redis_string.h
│   ├── redis_zset.cc
│   └── redis_zset.h
├── valgrind.sup
└── version.h.in

8 directories, 104 files

src/commands/redis_cmd.cc

torwig

@PragmaTwice Good job!
LGTM (with or without src/common among include directories)

tisonkun · 2022-10-16T06:14:33Z

@PragmaTwice please rebase on the latest unstable branch.

@ShooterIT @caipengbo could you give a review on this patch? Perhaps we should move it forward in a timely manner to avoid further conflicts.

caipengbo

LGTM

PragmaTwice · 2022-10-16T14:50:57Z

@PragmaTwice please rebase on the latest unstable branch.

Done.

PragmaTwice requested review from git-hulk, torwig, ShooterIT, tisonkun and caipengbo October 14, 2022 09:22

PragmaTwice linked an issue Oct 14, 2022 that may be closed by this pull request

Proposal: Refactor source file structure #977

Closed

2 tasks

tisonkun reviewed Oct 14, 2022

View reviewed changes

PragmaTwice force-pushed the refactor-source-structure branch from c3ede31 to 1d6a8ec Compare October 15, 2022 07:00

PragmaTwice requested a review from tisonkun October 15, 2022 07:03

git-hulk previously approved these changes Oct 15, 2022

View reviewed changes

tisonkun reviewed Oct 15, 2022

View reviewed changes

src/commands/redis_cmd.cc Show resolved Hide resolved

tisonkun previously approved these changes Oct 15, 2022

View reviewed changes

torwig previously approved these changes Oct 15, 2022

View reviewed changes

caipengbo previously approved these changes Oct 16, 2022

View reviewed changes

PragmaTwice added 4 commits October 16, 2022 22:39

Refactor source file structure

4e043ee

add stats dir

27485ba

move geohash to types

45dd408

network to server

1b6235a

format

e046efe

PragmaTwice dismissed stale reviews from caipengbo, torwig, tisonkun, and git-hulk via e046efe October 16, 2022 14:50

PragmaTwice force-pushed the refactor-source-structure branch from 1d6a8ec to e046efe Compare October 16, 2022 14:50

PragmaTwice requested review from caipengbo, tisonkun, git-hulk and torwig October 16, 2022 15:09

git-hulk approved these changes Oct 16, 2022

View reviewed changes

torwig approved these changes Oct 16, 2022

View reviewed changes

tisonkun approved these changes Oct 16, 2022

View reviewed changes

tisonkun merged commit 17d5fd7 into apache:unstable Oct 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor source file structure #989

Refactor source file structure #989

PragmaTwice commented Oct 14, 2022

PragmaTwice commented Oct 14, 2022

caipengbo commented Oct 14, 2022

PragmaTwice commented Oct 14, 2022

caipengbo commented Oct 14, 2022

PragmaTwice commented Oct 14, 2022

caipengbo commented Oct 14, 2022

PragmaTwice commented Oct 14, 2022

tisonkun left a comment

tisonkun Oct 14, 2022

PragmaTwice Oct 14, 2022

tisonkun Oct 14, 2022

PragmaTwice Oct 14, 2022 •

edited

Loading

tanruixiang commented Oct 14, 2022

git-hulk commented Oct 14, 2022

PragmaTwice commented Oct 15, 2022 •

edited

Loading

PragmaTwice commented Oct 15, 2022 •

edited

Loading

torwig left a comment

tisonkun commented Oct 16, 2022

caipengbo left a comment

PragmaTwice commented Oct 16, 2022

Refactor source file structure #989

Refactor source file structure #989

Conversation

PragmaTwice commented Oct 14, 2022

PragmaTwice commented Oct 14, 2022

caipengbo commented Oct 14, 2022

PragmaTwice commented Oct 14, 2022

caipengbo commented Oct 14, 2022

PragmaTwice commented Oct 14, 2022

caipengbo commented Oct 14, 2022

PragmaTwice commented Oct 14, 2022

tisonkun left a comment

Choose a reason for hiding this comment

tisonkun Oct 14, 2022

Choose a reason for hiding this comment

PragmaTwice Oct 14, 2022

Choose a reason for hiding this comment

tisonkun Oct 14, 2022

Choose a reason for hiding this comment

PragmaTwice Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

tanruixiang commented Oct 14, 2022

git-hulk commented Oct 14, 2022

PragmaTwice commented Oct 15, 2022 • edited Loading

PragmaTwice commented Oct 15, 2022 • edited Loading

torwig left a comment

Choose a reason for hiding this comment

tisonkun commented Oct 16, 2022

caipengbo left a comment

Choose a reason for hiding this comment

PragmaTwice commented Oct 16, 2022

PragmaTwice Oct 14, 2022 •

edited

Loading

PragmaTwice commented Oct 15, 2022 •

edited

Loading

PragmaTwice commented Oct 15, 2022 •

edited

Loading