deprecate RawNode.TickQuiesced()
#62
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @tbg @pavelkalinnikov |
pav-kv
approved these changes
May 30, 2023
|
@erikgrinaker Please add a Signed-off-by footer: @tbg Please approve/merge, as I can't. |
spzala
reviewed
May 30, 2023
|
@erikgrinaker thanks, and please take care of the DCO. |
spzala
requested changes
May 30, 2023
|
This technically breaks drop-in backwards compatibility. I still think we should remove this, as it's quite crufty. I used Github code search and there are 200+ hits, but as far as I can tell they're all in
Would appreciate some advice from the other maintainers. Personally, I think the benefit of removing this far outweigh the risks (in fact, I'd be concerned about someone using this). |
Thanks for the investigation @tbg I think we should at least mention this in the Changelog. This repo is still new so seems like we don't have the Changelog file created but it's time to do so now :) cc @ahrtr |
|
If we decide to remove a public API, we need to add a breaking change notice at least. Creating a changelog (thx @spzala for the reminder) is a good idea. I am also thinking to create a milestone as a goal to release next raft version, e.g. 3.6.0. Detailed proposals:
|
erikgrinaker
force-pushed
the
remove-tick-quiesced
branch
from
aedf16f
to
2cedf9c
Compare
|
Thanks for the code search @tbg! I'd be surprised and concerned if someone was using this. I don't know what our guarantees are around backwards compatibility, but it makes sense to leave this for either 3.6 or 4.0. This is not a priority at all, just an opportunistic cleanup. A milestone and changelog sounds good, let me know if we can help set that up.
✅ |
|
Usually a best practice is to deprecate a public API firstly (e.g in 3.6) to discourage users from using it, and then remove it in a major release (e.g. 4.0). So two options:
|
erikgrinaker
force-pushed
the
remove-tick-quiesced
branch
from
2cedf9c
to
4d42534
Compare
erikgrinaker
changed the title
RawNode.TickQuiesced()RawNode.TickQuiesced()
|
Sure, updated the PR to deprecate it and opened #66 to track the removal in the next major release. |
`TickQuiesced()` was originally added for use with range quiescence in CockroachDB, but hasn't been in use since 2018. This patch marks it as deprecated, to be removed in a future release. Signed-off-by: Erik Grinaker <grinaker@cockroachlabs.com>
erikgrinaker
force-pushed
the
remove-tick-quiesced
branch
from
4d42534
to
d6c1d64
Compare
|
TFTRs! Maybe we should add a 4.0 milestone too, and track #66 in it? |
Done. |
nvanbenschoten
added a commit
to nvanbenschoten/cockroach
that referenced
this pull request
May 21, 2024
This commit removes the `(*RawNode).TickQuiesced` method. The method was deprecated back in etcd-io/raft#62 and has not been in use since 2018. Epic: None Release note: None
craig bot
pushed a commit
to cockroachdb/cockroach
that referenced
this pull request
May 21, 2024
123120: ui: Highlight unavailable ranges in red on the summary bar with nonzero r=abarganier a=theloneexplorerquest Modify the summary bar to change the color of unavailable ranges. When the unavailable range is greater than zero, it will be displayed in red; if it is zero, it will be green. Fix: #122014 Release note (ui): Changed the color of unavailable ranges on the summary bar to red when nonzero; ranges are green when zero. 124301: logtestutils: generalize structured logging spy r=xinhaoz a=xinhaoz This commit generalizes the structured logging spy previously being used for datadriven telemetry tests so that it can repurposed for other structured logging channels. Epic: none Release note: None 124403: roachtest: use first transient error when checking for flakes r=srosenberg a=renatolabs Previously, roachtest would only look at the outermost error in a chain that matched a `TransientError` (or `ErrorWithOwnership`) when checking for flakes. However, that is in most cases *not* what we want: if a transient error wraps another transient error, the actual reason for the failure is the original (wrapped) error. Informs: #123887 Release note: None 124425: pkg/server/structlogging: support hot ranges stats with diagnostic reporting disabled r=kyle-a-wong a=kyle-a-wong Previously, enable hot ranges stats also required the enabling of diagnostic reporting. Hot ranges stats doesn't need to be dependent on diagnostic reporting and someone might want to enable hot ranges stats without enabling diagnostic reporting. Now, server.telemetry.hot_ranges_stats.enabled can be set to true while without setting diagnostics.reporting.enabled Epic: none Fixes: #122977 Part of: https://cockroachlabs.atlassian.net/browse/CRDB-38152 Release note: None 124459: builtins: fix st_geojson when max_decimal_digits is specified r=yuzefovich a=yuzefovich This commit fixes a regression in `st_geojson` builtin when `max_decimal_digits` argument is specified which was introduced in 6009141 (during 24.1 cycle). In particular, this overload specifies the precision (rather than using the default number of digits), and that commit made it so that we ignore the precision argument. This is now fixed. Fixes: #124368. Release note (bug fix): CockroachDB previously would ignore `max_decimal_digits` argument of `st_geojson` builtin function and would use the default instead. The bug is only present in 24.1.0 releases. 124491: raft: remove RawNode.TickQuiesced r=pav-kv a=nvanbenschoten This commit removes the `(*RawNode).TickQuiesced` method. The method was deprecated back in etcd-io/raft#62 and has not been in use since 2018. Epic: None Release note: None Co-authored-by: theloneexplorerquest <theloneexplorerquest@gmail.com> Co-authored-by: Xin Hao Zhang <xzhang@cockroachlabs.com> Co-authored-by: Renato Costa <renato@cockroachlabs.com> Co-authored-by: Kyle Wong <kyle.wong@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
craig bot
pushed a commit
to cockroachdb/cockroach
that referenced
this pull request
May 21, 2024
119416: pkg/util/eventagg: general aggregation framework for reduction of event cardinality r=dhartunian a=abarganier
**Reviewer note: review commit-wise**
The eventagg package is (currently) a proof of concept ("POC") that aims to provide an easy-to-use library that standardizes the way in which we aggregate Observability event data in CRDB. The goal is to eventually emit that data as "exhaust" from CRDB, which downstream systems can consume to build Observability features that do not rely on CRDB's own availability to aid in debugging & investigations. Additionally, we want to provide facilities for code within CRDB to consume this same data, such that it can also power features internally.
This pull request contains work to create the aggregation mechanism in `pkg/util/eventagg`.
This facilities provide a way of aggregating notable events to reduce cardinality, before performing further processing and/or structured logging.
In addition to the framework, a toy SQL Stats example is provided in `pkg/sql/sqlstats/aggregate.go`, which shows the current developer experience when using the APIs.
See `pkg/util/eventagg/doc.go` for more details
Since this feature is currently experimental, it's gated by the `COCKROACH_ENABLE_STRUCTURED_EVENTS` environment variable, which is disabled by default.
---
Release note: none
Epic: CRDB-35919
123120: ui: Highlight unavailable ranges in red on the summary bar with nonzero r=abarganier a=theloneexplorerquest
Modify the summary bar to change the color of unavailable ranges. When the unavailable range is greater than zero, it will be displayed in red; if it is zero, it will be green.
Fix: #122014
Release note (ui): Changed the color of unavailable ranges on the summary bar to red when nonzero; ranges are green when zero.
124160: roachtest: add test for admission control disk bandwidth r=sumeerbhola a=aadityasondhi
This test runs a single node target cluster that has two workloads
running on it. The lower priority (qos=background) is very bandwidth
intensive, and without the AC bandwidth limiter would saturate the
provisioned bandwidth (controlled using cgroups).
This test shows how setting the cluster setting
`kvadmission.store.provisioned-bandwidth` limits the disk bandwidth
usage of lower priority work and shapes it at the value set in the
setting.
Fixes #121576.
Release note: None
124293: tools: switch md5 cmd name based on existence r=dt a=dt
Release note: none.
Epic: none.
124348: backupccl: download pre restore data in cluster restore r=dt a=msbutler
This patch adds the pre restore data spans to the list of spans to download.
While these pre restore spans map to data in the temporary system table
database that are then rewwritten to the actual system table, the download job
ought to download all external data linked into the cluster out of principle.
Fixes #124330
Release note: none
124403: roachtest: use first transient error when checking for flakes r=srosenberg a=renatolabs
Previously, roachtest would only look at the outermost error in a chain that matched a `TransientError` (or `ErrorWithOwnership`) when checking for flakes. However, that is in most cases *not* what we want: if a transient error wraps another transient error, the actual reason for the failure is the original (wrapped) error.
Informs: #123887
Release note: None
124486: kvclient: add WithFiltering option to rangefeed client r=nvanbenschoten,msbutler a=stevendanna
This adds a WithFiltering option to the rangefeed client that passes through the option to the underlying rangefeed.
Epic: none
Release note: None
124491: raft: remove RawNode.TickQuiesced r=pav-kv a=nvanbenschoten
This commit removes the `(*RawNode).TickQuiesced` method. The method was deprecated back in etcd-io/raft#62 and has not been in use since 2018.
Epic: None
Release note: None
Co-authored-by: Alex Barganier <abarganier@cockroachlabs.com>
Co-authored-by: theloneexplorerquest <theloneexplorerquest@gmail.com>
Co-authored-by: Aaditya Sondhi <20070511+aadityasondhi@users.noreply.github.com>
Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Co-authored-by: Michael Butler <butler@cockroachlabs.com>
Co-authored-by: Renato Costa <renato@cockroachlabs.com>
Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TickQuiesced()was originally added for use with range quiescence in CockroachDB, but hasn't been in use since 2018. This patch marks it as deprecated, to be removed in a future release.See cockroachdb/cockroach#24956.