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.

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

Features/actor lineage #4136

Merged
merged 131 commits into from
May 3, 2021

Conversation

sfc-gh-mpilman
Copy link
Collaborator

This PR is resolves #...

Changes in this PR:

  • Adds the actor linage feature as described here.

General guideline:

  • If this PR is ready to be merged (and all checkboxes below are either ticked or not applicable), make this a regular PR
  • If this PR still needs work, please make this a draft PR
    • If you wish to get feedback/code-review, please add the label RFC to this PR

Please verify that all things listed below were considered and check them. If an item doesn't apply to this type of PR (for example a documentation change doesn't need to be performance tested), you should make a strikethrough (markdown syntax: ~~strikethrough~~). More infos on the guidlines can be found here.

Style

  • All variable and function names make sense.
  • The code is properly formatted (consider running git clang-format).

Performance

  • All CPU-hot paths are well optimized.
  • The proper containers are used (for example std::vector vs VectorRef).
  • There are no new known SlowTask traces.

Testing

  • The code was sufficiently tested in simulation.
  • If there are new parameters or knobs, different values are tested in simulation.
  • ASSERT, ASSERT_WE_THINK, and TEST macros are added in appropriate places.
  • Unit tests were added for new algorithms and data structure that make sense to unit-test
  • If this is a bugfix: there is a test that can easily reproduce the bug.

flow/flow.h Outdated
// A user should implement this for any type
// within the properies class.
template<class Value>
bool isSet(Value Derived::*member) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this function ever be called?

this has to be implemented by subclasses

Makes me wonder if it shouldn't ever be called, in which case maybe we can ASSERT(false) here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably - I still wonder how we could provide a default implementation for this... But all solutions involve a lot of template-code (and I don't want to do that in flow.h...)

static StringRef name;
ProcessClass::ClusterRole role = ProcessClass::NoRole;

bool isSet(ProcessClass::ClusterRole RoleLineage::*member) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this syntax. Do you have a reference I could look at for this (I'm not exactly sure what to google). What type does member have?

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 is the pointer to member syntax (we use this in loadBalance). The only thing I can find is MS documentation: https://docs.microsoft.com/en-us/cpp/cpp/pointer-to-member-operators-dot-star-and-star?view=msvc-160 (but if you google it you can also find some stack overflow questions that are useful)

sfc-gh-ljoswiak and others added 12 commits April 28, 2021 15:04
Fixes the following issues:

1. Use the right index when initializing the WriteOnlySet's vector of
   atomics. Also switch to std::atomic_init to initialize each atomic in
   the vector (cannot default construct the atomics in the vector
   because std::atomic does not have a copy constructor).
2. Add failure check for when items cannot be inserted into the
   WriteOnlySet due to capacity constraints. This situation occurs when
   `copy` is not called on the WriteOnlySet, such as when sampling is
   disabled. The `copy` function is what clears the WriteOnlySet.
3. Remove a global config feature I added to update the ClientDBInfo
   object used by the global config listener function. This needs more
   investigation, but the effect of this change could be that global
   config changes are not correctly recognized on fdbserver processes.
4. Add various ASSERTs to verify data in WriteOnlySet.
This is causing problems with the 5.2.0 restarting test. Removing this
line disables fdbserver processes from receiving global config updates,
instead requiring a restart to see them.
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 20ab65a
  • Result: FAILED
  • Build Logs (available for 7 days)

@sfc-gh-ljoswiak sfc-gh-ljoswiak marked this pull request as ready for review May 2, 2021 21:13
@sfc-gh-ljoswiak sfc-gh-ljoswiak self-assigned this May 2, 2021
@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 8dcd779
  • Result: FAILED
  • Build Logs (available for 7 days)

@foundationdb-ci
Copy link
Contributor

AWS CodeBuild CI Report

  • CodeBuild project: foundationdb-pull-request-build
  • Commit ID: 1c1125c
  • Result: SUCCEEDED
  • Build Logs (available for 7 days)

@sfc-gh-ljoswiak sfc-gh-ljoswiak merged commit da41534 into apple:master May 3, 2021
sfc-gh-ljoswiak added a commit to sfc-gh-ljoswiak/foundationdb that referenced this pull request May 11, 2021
…tor-lineage"

This reverts commit da41534, reversing
changes made to e630090.
sfc-gh-ljoswiak added a commit to sfc-gh-ljoswiak/foundationdb that referenced this pull request May 11, 2021
…tor-lineage"

This reverts commit da41534, reversing
changes made to e630090.
sfc-gh-ljoswiak added a commit that referenced this pull request May 12, 2021
…lease-7.0

Revert "Merge pull request #4136 from sfc-gh-mpilman/features/actor-l…
sfc-gh-ljoswiak added a commit that referenced this pull request May 12, 2021
Revert "Merge pull request #4136 from sfc-gh-mpilman/features/actor-l…
xis19 pushed a commit to xis19/foundationdb that referenced this pull request May 14, 2021
…tor-lineage"

This reverts commit da41534, reversing
changes made to e630090.
@sfc-gh-ljoswiak sfc-gh-ljoswiak mentioned this pull request Aug 2, 2021
5 tasks
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.

4 participants