Skip to content

Make TCInfo.h private in fdbserver/datadistributor#13404

Open
tclinkenbeard-oai wants to merge 1 commit into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/datadistributor-private-headers
Open

Make TCInfo.h private in fdbserver/datadistributor#13404
tclinkenbeard-oai wants to merge 1 commit into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/datadistributor-private-headers

Conversation

@tclinkenbeard-oai

Copy link
Copy Markdown
Collaborator

This PR is the first step in an effort to improve fdbserver/datadistributor encapsulation. Today fdbserver/datadistributor has many public header files, and this PR addresses some low-hanging fruit

@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?

Make TCInfo.h private to fdbserver/datadistributor by moving it out of the exported include tree. The PR replaces transitive dependencies with DataDistributionTeam.h, forward-declares TC types in DDTeamCollection.h, and moves UID-based addTeam helpers into DDTeamCollection.actor.cpp.

Is it correct?

Yes, on inspection. The TCInfo.h contents are unchanged, all in-tree includes of the old path are updated, and the datadistributor target already has private access to its source directory. The new vector/set overloads preserve filtering of unknown UIDs, sorted ordering, and empty-team behavior, and all former iterator-pair call sites are converted.

There are no serialized, persisted, protocol, status, or async behavior changes. No build or test validation was run.

Visible CI: clang-format is green; Test Boost CONFIG Mode on Windows is in progress; the FoundationDB PR Builder, clang, clang arm, clang-ide, and Cluster Tests checks are pending. No failing checks are visible.

Are there bugs?

I did not find any correctness bugs.

Are there omissions?

No blocking omissions. Existing DDTeamCollection unit-test code still calls the set overload. The remaining risk is compile-time header self-containment, which the pending builders should cover.

Are there better ways of doing things?

None that seem worthwhile. The out-of-line overloads plus forward declarations are the smallest way to make TCInfo private without changing behavior.

Should this CL be LGTMd?

Yes, LGTM on code inspection. I inspected the patch, exact-base include consumers, addTeam call paths, public review threads, and current CI state. Merge should wait for the pending builders, with header self-containment as the highest remaining risk.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

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

  • Commit ID: 559caa1
  • Duration 0:21:21
  • 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: 559caa1
  • Duration 0:34:00
  • 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: 559caa1
  • Duration 0:44:53
  • 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: 559caa1
  • Duration 0:47:33
  • 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: 559caa1
  • Duration 0:54:41
  • 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: 559caa1
  • Duration 0:57:52
  • 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: 559caa1
  • Duration 1:06:49
  • 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