Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split DataDistribution.actor.cpp into multiple files #6342

Merged
merged 11 commits into from
Feb 9, 2022
Merged

Split DataDistribution.actor.cpp into multiple files #6342

merged 11 commits into from
Feb 9, 2022

Conversation

sfc-gh-tclinkenbeard
Copy link
Collaborator

This addresses #3603. This change can help with incremental compilation time and help to decouple the interfaces and implementations of various components of data distribution. There are future changes to come (to improve encapsulation, const-correctness, etc.), but this one will have the most merge conflicts, so I am creating this pull request first to avoid future conflicts.

Code-Reviewer Section

The general guidelines can be found here.

Please check each of the following things and check all boxes before accepting a PR.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or master if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

@sfc-gh-tclinkenbeard
Copy link
Collaborator Author

This passed 100k simulation tests: 20220204-190039-tclinkenbeard-a4e2487cf4c07e9c.

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 6ab4bc0
  • Result: FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: 6ab4bc0
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

sfc-gh-bvr
sfc-gh-bvr reviewed Feb 4, 2022
View reviewed changes
fdbserver/DDTeamCollection.actor.cpp Outdated
*
* This source file is part of the FoundationDB open source project
*
* Copyright 2013-2018 Apple Inc. and the FoundationDB project authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be updated manually? Or is there an automated process? Or does it not matter? Same question for the .h

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs to be added manually for each new file.

fdbserver/DDTeamCollection.h
#include "flow/BooleanParam.h"
#include "flow/Trace.h"
#include "flow/UnitTest.h"
#include "flow/actorcompiler.h" // This must be the last #include.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious. Is this list of includes tested for necessity somehow, using IWYU or some such?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, there is no automated testing of the necessity of these includes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

AFAIK no one has looked into using IWYU to clean up the includes. It would be nice to use, but it may be tricky to get working with the actor compiler.

fdbserver/DDTeamCollection.actor.cpp
return Reference<TCMachineTeamInfo>();
}

void DDTeamCollection::traceServerInfo() const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious: Why the different treatment for this trace method from other similar trace methods in the DDTeamCollection class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The get helper function in DDTeamCollection.actor.cpp is needed in the implementation here, so I had to move the implementation here in order to compile. In a future PR, I plan to move all non-trivial implementations out of the header file.

fdbserver/DDTeamCollection.actor.cpp Outdated Show resolved Hide resolved
fdbserver/TCInfo.h Outdated Show resolved Hide resolved
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: a2b4fb9
  • Result: FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: fd16207
  • Result: FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: ca7fb0b
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: ca7fb0b
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: ebf940b
  • Result: FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: 3d3223d
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 3d3223d
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

sfc-gh-xwang
sfc-gh-xwang previously approved these changes Feb 9, 2022
View reviewed changes
Copy link
Collaborator

@sfc-gh-xwang sfc-gh-xwang left a comment

Choose a reason for hiding this comment

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

LGTM.
XXXImpl class may be the best practice addressing the issue the ACTOR must be a static method and the static method must be defined within a class...

liquid-helium
liquid-helium previously approved these changes Feb 9, 2022
View reviewed changes
Copy link
Contributor

@liquid-helium liquid-helium left a comment

Choose a reason for hiding this comment

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

LGTM.

One slightly related question not limited to this PR. In many places in the code base, implementations code is written in the header files, instead of separate .cpp files, are there any reasons for this? IIUIC, the separation of .h and .cpp could also help improve the compile speed.

fdbserver/DDTeamCollection.actor.cpp Outdated Show resolved Hide resolved
@sfc-gh-tclinkenbeard
Copy link
Collaborator Author

One slightly related question not limited to this PR. In many places in the code base, implementations code is written in the header files, instead of separate .cpp files, are there any reasons for this? IIUIC, the separation of .h and .cpp could also help improve the compile speed.

The only reason for leaving the implementations in header files now is to avoid merge conflicts as much as possible. In a future PR the non-trivial implementations will be moved into cpp files.

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: dcbbee5
  • Result: FAILED
  • Error: Error while executing command: ninja -v -C build_output -j ${NPROC} all packages strip_targets. Reason: exit status 1
  • Build Logs (available for 30 days)

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@fdb-windows-ci
Copy link
Collaborator

Doxense CI Report for Windows 10

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for macOS BigSur 11.5.2

  • CodeBuild project: foundationdb-pr-macos
  • Commit ID: 3f0e2ae
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@sfc-gh-tclinkenbeard
Copy link
Collaborator Author

This passed another 100k tests after fixing merge conflicts: 20220209-223951-tclinkenbeard-25cdb0ecf4f1da1d.

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report for Linux CentOS 7

  • CodeBuild project: foundationdb-pr
  • Commit ID: 3f0e2ae
  • Result: SUCCEEDED
  • Error: N/A
  • Build Logs (available for 30 days)

@sfc-gh-tclinkenbeard sfc-gh-tclinkenbeard merged commit 84ef34b into apple:main Feb 9, 2022
@sfc-gh-tclinkenbeard sfc-gh-tclinkenbeard mentioned this pull request Feb 10, 2022
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liquid-helium liquid-helium liquid-helium approved these changes

@sfc-gh-bvr sfc-gh-bvr sfc-gh-bvr approved these changes

@sfc-gh-etschannen sfc-gh-etschannen Awaiting requested review from sfc-gh-etschannen

@sfc-gh-jslocum sfc-gh-jslocum Awaiting requested review from sfc-gh-jslocum

@sfc-gh-xwang sfc-gh-xwang Awaiting requested review from sfc-gh-xwang

Assignees

@sfc-gh-etschannen sfc-gh-etschannen

@liquid-helium liquid-helium

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sfc-gh-tclinkenbeard @foundationdb-ci @fdb-windows-ci @sfc-gh-xwang @liquid-helium @sfc-gh-bvr @sfc-gh-etschannen