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

[dbnode] Update return unfulfilled for corrupt commit log files default #2807

Merged

Conversation

robskillington
Copy link
Collaborator

@robskillington robskillington commented Oct 28, 2020

What this PR does / why we need it:

As any forced shutdown of the DB node (via SIGKILL instead of SIGTERM) causes the final commit log entry to be corrupt, all users today use default for this flag. Ahead of 1.0 we should update this value to reflect the usage of this everywhere.

For more information see the following:
https://github.com/m3db/m3/blob/64c6dea950ce27108841ecb82304cf8c46eea158/site/content/docs/operational_guide/availability_consistency_durability.md#ignoring-corrupt-commitlogs-on-bootstrap

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

NONE

Does this PR require updating code package or user-facing documentation?:

NONE

As any forced shutdown of the DB node (via SIGKILL instead of SIGTERM) causes the final commit log entry to be corrupt, all users today use default for this flag. Ahead of 1.0 we should update this value to reflect the usage of this everywhere.
Copy link
Collaborator

@nbroyles nbroyles left a comment

Choose a reason for hiding this comment

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

LGTM pending build fixes

@@ -34,7 +34,7 @@ const (
// DefaultReturnUnfulfilledForCorruptCommitLogFiles is the default
// value for whether to return unfulfilled when encountering corrupt
// commit log files.
DefaultReturnUnfulfilledForCorruptCommitLogFiles = true
DefaultReturnUnfulfilledForCorruptCommitLogFiles = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this the default we want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@robskillington robskillington merged commit 6cd0148 into master Oct 29, 2020
@robskillington robskillington deleted the r/return-unfulfilled-for-corrupt-commit-log-files branch October 29, 2020 19:31
soundvibe added a commit that referenced this pull request Oct 30, 2020
* master:
  [query] count_values label naming fixes (#2805)
  [dbnode] Read only namespaces (#2803)
  Remove old code: m3nsch (#2822)
  Update issue templates
  Update issue templates
  [proto] Allow zero-alloc reuse of AggregatedMetric protobuf payloads (#2823)
  Remove old code: aggregator/handler/trafficcontrol (#2821)
  Remove old code: aggregation/quantile/tdigest (#2820)
  AddToReset config to change Add transforms to Reset transforms (#2817)
  [metrics/rules] Add test to assert that mixed-mode keepOriginal uses keepOriginal=true (#2819)
  [dbnode] Update return unfulfilled for corrupt commit log files default (#2807)
  [dbnode] Move eviction logic up before ns loop (#2812)
  [query] Add additional parser tests (#2811)
  [config] Remove deprecated configuration fields (#2771)
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