Skip to content

Encapsulate several header files#13407

Open
tclinkenbeard-oai wants to merge 3 commits into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/encapsulation-private-headers
Open

Encapsulate several header files#13407
tclinkenbeard-oai wants to merge 3 commits into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/encapsulation-private-headers

Conversation

@tclinkenbeard-oai

Copy link
Copy Markdown
Collaborator

This PR helps improve encapsulation of several libraries by making the following header files private:

  • HealthMonitor.h
  • KnobProtectiveGroups.h
  • PartitionMapMessage.h

@tclinkenbeard-oai tclinkenbeard-oai left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Generated by Codex.

What is it trying to do?

This PR makes three implementation headers private: fdbrpc's HealthMonitor, backup worker's PartitionMapMessage, and tester's KnobProtectiveGroups. It preserves the needed public surface with FlowTransport::getRecentClosedPeers() and a new public KnobKeyValuePairs.h.

Is it correct?

Yes, based on code inspection. The HealthMonitor logic is unchanged; the new wrapper returns the same recent-closed-peer set, and the connection paths access the same TransportData-owned monitor. PartitionMapMessage is a pure header move with unchanged serialization, so its durable log representation is unchanged. KnobKeyValuePairs is moved intact into a self-contained public header, while private users use local includes. CMake source discovery still includes the moved headers in their respective targets.

No build or test validation was run for this review. At review time, clang-format is passing; Test Boost CONFIG Mode on Windows, the FoundationDB PR Builder variants, and FoundationDB CI - PR Cluster Tests are pending.

Are there bugs?

I did not find any correctness bugs. I checked serialization/persistence boundaries, hot-path work, async lifetime/cancellation, removed invariants, and test coverage implications.

Are there omissions?

None that I think block this. This is an encapsulation-only change, so new behavioral tests do not seem necessary; the remaining meaningful validation is compile/link coverage from the pending CI jobs.

Are there better ways of doing things?

One non-blocking wording tweak: FlowTransport::getRecentClosedPeers() says returned peers “have already been evicted from the transport peer map”, but the underlying monitor returns all recently closed peers and doPeerHealthCheck() performs the allPeers de-duplication. I would describe it as “peers with recent failed connections” or document the caller-side filtering.

Should this CL be LGTMd?

Yes, LGTM from code inspection. I inspected the header moves/split, health-monitor call paths and lifetime, CMake source discovery, and unchanged PartitionMapMessage serialization. The highest remaining risk is include/compile fallout from moving public headers, so I would wait for the pending CI checks before merging.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: beb673a
  • Duration 0:20:12
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos-m1 on macOS 14.x

  • Commit ID: beb673a
  • Duration 0:31:28
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-arm on Linux RHEL 9

  • Commit ID: beb673a
  • Duration 0:45:16
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS 14.x

  • Commit ID: beb673a
  • Duration 0:46:56
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: beb673a
  • Duration 0:54:43
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: beb673a
  • Duration 0:57:04
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: beb673a
  • Duration 1:04:08
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

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