-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Cassandra 18490 Fix checksum validation and checksum validate indexes streaming and SSTable import #2460
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
Cassandra 18490 Fix checksum validation and checksum validate indexes streaming and SSTable import #2460
Changes from all commits
38d2136
af19656
1c83022
db24df5
479b070
76bc6cb
e1550b9
5f7f894
ac0148a
c4edd04
ce5f73b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ | |
| # | ||
|
|
||
| BASEDIR=`dirname $0` | ||
| BASE_BRANCH=trunk | ||
| BASE_BRANCH=cep-7-sai | ||
| set -e | ||
|
|
||
| die () | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.cassandra.index.sai; | ||
|
|
||
| public enum IndexValidation | ||
| { | ||
| /** | ||
| * No validation to be performed | ||
| */ | ||
| NONE, | ||
|
|
||
| /** | ||
| * Basic header/footer validation, but no data validation (fast) | ||
| */ | ||
| HEADER_FOOTER, | ||
|
|
||
| /** | ||
| * Full validation with checksumming data (slow) | ||
| */ | ||
| CHECKSUM | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,8 +261,9 @@ private CountDownLatch shouldWritePerSSTableFiles(SSTableReader sstable) | |
| { | ||
| // if per-table files are incomplete or checksum failed during full rebuild. | ||
| IndexDescriptor indexDescriptor = IndexDescriptor.create(sstable); | ||
| if (!indexDescriptor.isPerSSTableIndexBuildComplete() || | ||
| (isFullRebuild && !indexDescriptor.validatePerSSTableComponentsChecksum())) | ||
| if (!indexDescriptor.isPerSSTableIndexBuildComplete() | ||
| || isFullRebuild | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we initially wrote this, we couldn't think of any cases where the SSTable-level indexes would need to be rebuilt if existing and valid. The column-level bits are typically what need to change, say if you slightly changed filtering/analysis on a text field. However, I'm not opposed to this if you think it's clearer that "full rebuild" really means "full" rebuild :) |
||
| || !indexDescriptor.validatePerSSTableComponents(IndexValidation.CHECKSUM)) | ||
| { | ||
| CountDownLatch latch = CountDownLatch.newCountDownLatch(1); | ||
| if (inProgress.putIfAbsent(sstable, latch) == null) | ||
|
|
@@ -307,7 +308,7 @@ private void completeSSTable(SSTableFlushObserver indexWriter, | |
|
|
||
| // register custom index components into existing sstables | ||
| sstable.registerComponents(StorageAttachedIndexGroup.getLiveComponents(sstable, existing), tracker); | ||
| Set<StorageAttachedIndex> incomplete = group.onSSTableChanged(Collections.emptyList(), Collections.singleton(sstable), existing, false); | ||
| Set<StorageAttachedIndex> incomplete = group.onSSTableChanged(Collections.emptyList(), Collections.singleton(sstable), existing, IndexValidation.NONE); | ||
|
|
||
| if (!incomplete.isEmpty()) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -250,15 +250,15 @@ public void handleNotification(INotification notification, Object sender) | |
| SSTableAddedNotification notice = (SSTableAddedNotification) notification; | ||
|
|
||
| // Avoid validation for index files just written following Memtable flush. | ||
| boolean validate = !notice.memtable().isPresent(); | ||
| IndexValidation validate = notice.memtable().isPresent() ? IndexValidation.NONE : IndexValidation.CHECKSUM; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this is the crux of the change, correct? (i.e. SSTables added via streaming or import will be checksummed, unlike before, where they would undergo only header/footer validation?)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One of the general things I looked at while reviewing here was whether the notification order of the different 1.) When we stream entire SSTables w/ their attached index components (or locally import new SSTables w/ index components already built), SIM handles the notification, but the index build it triggers should be a no-op. Even if it built something it wouldn't register as a full rebuild and no checksumming would be done. Then when 2.) When we don't stream an entire SSTable during repair or partial SSTable bootstrapping (or locally import new SSTables w/ index components not already built), SIM handles the notification and blocks to build SSTable-attached index bits. Then (again, this is an accident of our ordering) I'm not sure how exactly we want to address this, or whether we would want to address it in this patch (although that might be nice, since we have it loaded into our working memories), but perhaps we could agree that it's an issue (or not, and that I'm missing something) and go from there? Two things that come to mind to "fix" this are a.) having more information in the notification or b.) having the index group handle notifications before SIM does. (SIM only handles
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have been looking at this in some detail today to try and establish the best way to correct it. From our perspective, the The problem with the current implementation is:
What should be noted here is that the SIM will only invoke an index build if a memtable is not present in the notification, and the SAIG currently only validates the index components if the memtable isn't present but still registers the components in either case. We feel that the following changes could be made to improve this:
This will require some rework of the index builder, to make sure that the SSTables are always registered correctly and, as such, we would prefer that this was done on a separate ticket from this one. Which phase that ticket goes into is up for discussion. The downsides of the current implementation are that we can rebuild indexes that already have valid components and we can end up validating components twice. WDYT?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mike-tr-adamson @pkolaczk I'm okay w/ doing this in another Jira. There's enough risk to make sure we focus on it specifically, and after this patch we'll only be doing something sub-optimally, not incorrectly. Now...in solution space...
I don't see any problem w/ this, although I'm not sure I understand the problem in the streaming context.
Right after streaming, no index components for the new SSTables are present, right?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See CASSANDRA-18653 |
||
| onSSTableChanged(Collections.emptySet(), notice.added, indexes, validate); | ||
| } | ||
| else if (notification instanceof SSTableListChangedNotification) | ||
| { | ||
| SSTableListChangedNotification notice = (SSTableListChangedNotification) notification; | ||
|
|
||
| // Avoid validation for index files just written during compaction. | ||
| onSSTableChanged(notice.removed, notice.added, indexes, false); | ||
| onSSTableChanged(notice.removed, notice.added, indexes, IndexValidation.NONE); | ||
| } | ||
| else if (notification instanceof MemtableRenewedNotification) | ||
| { | ||
|
|
@@ -298,9 +298,9 @@ void dropIndexSSTables(Collection<SSTableReader> ss, StorageAttachedIndex index) | |
| * files being corrupt or being unable to successfully update their views | ||
| */ | ||
| synchronized Set<StorageAttachedIndex> onSSTableChanged(Collection<SSTableReader> removed, Iterable<SSTableReader> added, | ||
| Set<StorageAttachedIndex> indexes, boolean validate) | ||
| Set<StorageAttachedIndex> indexes, IndexValidation validation) | ||
| { | ||
| Pair<Set<SSTableContext>, Set<SSTableReader>> results = contextManager.update(removed, added, validate); | ||
| Pair<Set<SSTableContext>, Set<SSTableReader>> results = contextManager.update(removed, added, validation); | ||
|
|
||
| if (!results.right.isEmpty()) | ||
| { | ||
|
|
@@ -321,7 +321,7 @@ synchronized Set<StorageAttachedIndex> onSSTableChanged(Collection<SSTableReader | |
|
|
||
| for (StorageAttachedIndex index : indexes) | ||
| { | ||
| Collection<SSTableContext> invalid = index.getIndexContext().onSSTableChanged(removed, results.left, validate); | ||
| Collection<SSTableContext> invalid = index.getIndexContext().onSSTableChanged(removed, results.left, validation); | ||
|
|
||
| if (!invalid.isEmpty()) | ||
| { | ||
|
|
@@ -410,8 +410,8 @@ public SSTableContextManager sstableContextManager() | |
| public void unsafeReload() | ||
| { | ||
| contextManager.clear(); | ||
| onSSTableChanged(baseCfs.getLiveSSTables(), Collections.emptySet(), indexes, false); | ||
| onSSTableChanged(Collections.emptySet(), baseCfs.getLiveSSTables(), indexes, true); | ||
| onSSTableChanged(baseCfs.getLiveSSTables(), Collections.emptySet(), indexes, IndexValidation.NONE); | ||
| onSSTableChanged(Collections.emptySet(), baseCfs.getLiveSSTables(), indexes, IndexValidation.HEADER_FOOTER); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -422,6 +422,6 @@ public void reset() | |
| { | ||
| contextManager.clear(); | ||
| indexes.forEach(StorageAttachedIndex::makeIndexNonQueryable); | ||
| onSSTableChanged(baseCfs.getLiveSSTables(), Collections.emptySet(), indexes, false); | ||
| onSSTableChanged(baseCfs.getLiveSSTables(), Collections.emptySet(), indexes, IndexValidation.NONE); | ||
| } | ||
| } | ||
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.
nit: Comment needs to change slightly now, something like...