-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
storage: remove replica pointer tag #15212
storage: remove replica pointer tag #15212
Conversation
@@ -513,8 +513,6 @@ func newReplica(rangeID roachpb.RangeID, store *Store) *Replica { | |||
r.rangeStr.store(0, &roachpb.RangeDescriptor{RangeID: rangeID}) | |||
// Add replica log tag - the value is rangeStr.String(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so comments, such useful. amaze.
@tamird Those tests failures are really scary:
We didn't upgrade RocksDB recently, did we? So what changed? |
27193ee
to
c8d6697
Compare
@jordanlewis just reported this failure as well. We did not, and I'm not
sure. I won't have time to investigate until this afternoon due to the
all-hands. Perhaps @benesch has a minute to spend on this.
…On Thu, Apr 20, 2017 at 12:32 PM, Peter Mattis ***@***.***> wrote:
@tamird <https://github.com/tamird> Those tests failures are really scary:
cli.test: /go/src/github.com/cockroachdb/cockroach/c-deps/rocksdb.src/db/inlineskiplist.h:424: int rocksdb::InlineSkipList<Comparator>::RandomHeight() [with Comparator = const rocksdb::MemTableRep::KeyComparator&]: Assertion `height <= kMaxHeight_' failed.
We didn't upgrade RocksDB recently, did we? So what changed?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15212 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPLfkOIFSMgv84QKkVDPQ6U3cn5Miks5rx4iLgaJpZM4NDRXt>
.
|
Current suspicion is that #15188 is the culprit. Attempting to reproduce. |
Updated theory: this is probably build skew. We don't bust c-deps caches when we change |
Is there a particular motivation for removing this? It's also been useful when looking into replica GC bugs. |
Nuked the affected agent; all seems well. I'll work on a way to force rebuilding of C dependencies when |
I thought those bugs were well in the past. Am I mistaken about that? |
And the motivation was a minor amount of log cleanliness. |
LGTM. I noticed this and was thinking about removing it too. |
They are. I was just pointing out that it's been useful for more than just the original issue. It's not a huge deal to get rid of it again though, we can re-add it as needed. |
I could also comment it out. Preference? |
Commenting it out sgtm |
The replica pointer tag was used for debugging cockroachdb#11591, which has been long since fixed.
c8d6697
to
a5bbfcf
Compare
Done. |
The replica pointer tag was used for debugging #11591, which has been
long since fixed.