Cassandra 17774 #1751
Cassandra 17774 #1751
Conversation
There was a problem hiding this comment.
Will go ahead and fix the other MBean methods that are missing it as well. 👍
There was a problem hiding this comment.
I know we recommend 20-30 buckets, but if there are 31, are we just going to silently cut off the oldest one?
There was a problem hiding this comment.
Yes. We could up this, make it a configurable, both, neither, ? Nothing jumps out at me as an immediately convenient way to indicate to the end user that the value has been truncated when hitting up the JMX endpoint, and since we're not including it in the tablestats output we don't have that outlet to indicate truncation. You have any clever ideas here? :)
There was a problem hiding this comment.
The only easy improvement I can think of is to raise it to something that won't truncate w/ a slight misconfiguration. 128 maybe? Failing the JMX call because there are too many buckets seems like a bad option...
@krummas Thoughts?
There was a problem hiding this comment.
nit: Any way we can indent the concatenated streams here, so it's easier to read onward to filter?
There was a problem hiding this comment.
Ew. Good catch.
There was a problem hiding this comment.
if sstableCountByBuckets is accessed directly by a JMX thread, I think we want to make it volatile here.
There was a problem hiding this comment.
Subtle; good catch. This definitely opens this up to being accessed by multiple thread contexts. This tweak should give us slightly more up-to-date information in what we see via JMX if it just so happens to race w/the repair manager or compaction managers picking their next tasks. 👍
There was a problem hiding this comment.
Given its temporality, I might be inclined to want to read oldest to newest left to right. Is the thought here to emulate what we do w/ LCS where the levels move left to right? (i.e. The age of the data moves left to right?)
There was a problem hiding this comment.
I can't speak to the intent of the original author (will update commit message to reflect that it was Stefan's work; can check w/him), however the current shape of things is recency left to right, levels left to right, level 0 corresponding to "newest".
array index corresponds to level(int[0] is for level 0, ...)
array index corresponds to bucket(int[0] is for most recent, ...).
I don't really have an opinion on it excepting the current formatting provides logical symmetry between the two JMX calls and I like consistency even at the expense of intuition sometimes. 🤷
There was a problem hiding this comment.
I'm fine with keeping it like this.
Patch by Stefan Podkowinski; reviewed by CalebRackliffe and Marcus Eriksson for CASSANDRA-17774 Co-authored-by: Stefan Podkowinski <s.podkowinski@gmail.com> Co-authored-by: Josh McKenzie <jmckenzie@apache.org>
jmckenzie-dev
force-pushed
the
CASSANDRA-17774
branch
from
d63a4eb to
d784a82
Compare
- **Add failing test** - **CNDB-14153: Fix SAI updates (non-null solution)** ### What is the issue Fixes riptano/cndb#14153 ### What does this PR fix and why was it fixed This is meant as an alternative to datastax#1749. It fixes riptano/cndb#14153 by never returning `null` from the `UpsertTransformer`. apache#1749 is a more memory efficient solution, but has additional complexity, which is why I am proposing this as an alternative.
- **Add failing test** - **CNDB-14153: Fix SAI updates (non-null solution)** ### What is the issue Fixes riptano/cndb#14153 ### What does this PR fix and why was it fixed This is meant as an alternative to datastax#1749. It fixes riptano/cndb#14153 by never returning `null` from the `UpsertTransformer`. apache#1749 is a more memory efficient solution, but has additional complexity, which is why I am proposing this as an alternative.
2 participants
No description provided.