Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Date: Thu, 1 Jun 2017 17:53:04 +0000 (UTC)
From: =?utf-8?Q?Andr=C3=A9s_de_la_Pe=C3=B1a_=28JIRA=29?= <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12857283.1440006050000.343712.1496339584676@Atlassian.JIRA>
In-Reply-To: <JIRA.12857283.1440006050000@Atlassian.JIRA>
References: <JIRA.12857283.1440006050000@Atlassian.JIRA> <JIRA.12857283.1440006050745@jira-lw-us.apache.org>
Subject: [jira] [Commented] (CASSANDRA-10130) Node failure during 2i update
 after streaming can have incomplete 2i when restarted
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
archived-at: Thu, 01 Jun 2017 17:53:10 -0000


    [ https://issues.apache.org/jira/browse/CASSANDRA-10130?page=3Dcom.atla=
ssian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=
=3D16033389#comment-16033389 ]=20

Andr=C3=A9s de la Pe=C3=B1a commented on CASSANDRA-10130:
-----------------------------------------------

Here is the updated version of the patch:

||[trunk|https://github.com/apache/cassandra/compare/trunk...adelapena:1013=
0-trunk-memtable]|[utests|http://cassci.datastax.com/view/Dev/view/adelapen=
a/job/adelapena-10130-trunk-memtable-testall/]|[dtests|http://cassci.datast=
ax.com/view/Dev/view/adelapena/job/adelapena-10130-trunk-memtable-dtest/]|

bq. I personally like the "memtable" version more, as logically speaking, i=
f an sstable is added with a memtable, it means it has been also indexed, w=
hile in all other cases it means the sstable(s) have been externally loaded=
 and needs indexing

The only exception to this seems to be [{{CompactionStress}}|https://github=
.com/apache/cassandra/blob/trunk/tools/stress/src/org/apache/cassandra/stre=
ss/CompactionStress.java#L147]. It calls to {{ColumnFamilyStore#addSSTables=
}} without memtable and without indexing. Fortunately there shouldn't be an=
y associated 2i when the call is done, I have added [a check|https://github=
.com/adelapena/cassandra/blob/bb1f80b0b7be5122887621878fd6137627f72558/tool=
s/stress/src/org/apache/cassandra/stress/CompactionStress.java#L148] to mak=
e sure that it is so.

bq. 1) There are too many overloads of addSSTable(s), and some of them do n=
ot make much sense and are there just to support self calls (i.e. the versi=
on with many sstables and a single memtable I believe), so can we clean tha=
t up?

Cleaned.

bq. 2) Who is actually calling the addSSTable method with the memtable? I t=
hink I'm missing where it's actually used...

This is called from [{{Tracker#replaceFlushed}}|https://github.com/adelapen=
a/cassandra/blob/bb1f80b0b7be5122887621878fd6137627f72558/src/java/org/apac=
he/cassandra/db/lifecycle/Tracker.java#L340-L371], and it is checked in [{{=
TrackerTest#testMemtableReplacement}}|https://github.com/adelapena/cassandr=
a/blob/bb1f80b0b7be5122887621878fd6137627f72558/test/unit/org/apache/cassan=
dra/db/lifecycle/TrackerTest.java#L312].

bq. 3) Why did you reduce the test coverage in {{indexCorrectlyMarkedAsBuil=
dAndRemoved}}?

Because it seemed covered by dtests and simulating an index building failur=
e seemed harder without the function passed as parameter. But really there =
is a case that is not covered by dtests and the failure can be simulated pa=
ssing a null sstable collections, so I have added [an equivalent check|http=
s://github.com/adelapena/cassandra/blob/bb1f80b0b7be5122887621878fd6137627f=
72558/test/unit/org/apache/cassandra/index/internal/CassandraIndexTest.java=
#L512] using rebuilding.

bq. It would be nice if you could add a short comment explaining when the m=
emtable is present or not.

I have just added [several comments|https://github.com/adelapena/cassandra/=
blob/bb1f80b0b7be5122887621878fd6137627f72558/src/java/org/apache/cassandra=
/notifications/SSTableAddedNotification.java], not sure about if they are c=
lear enough or too repetitive.

bq. I'm not sure we should restrict the {{SSTableLoadedNotification}} to on=
ly loaded sstables, since a subscriber may want to do an action before and =
after sstables are added to the tracker, regardless if they are flushed or =
externally loaded, so we should probably trigger regardless if memtable is =
null or not and rename the notification to {{SSTableBeforeAddedNotification=
}} (or similar) - this will also prevent confusion from users thinking the =
sstables are already in the tracker when receiving an {{SSTableLoadedNotifi=
cation}}.

Good idea. I have replaced {{SSTableLoadedNotification}} by a [{{SSTableBef=
oreAddedNotification}}|https://github.com/adelapena/cassandra/blob/bb1f80b0=
b7be5122887621878fd6137627f72558/src/java/org/apache/cassandra/notification=
s/SSTableBeforeAddedNotification.java] that is always send before {{SSTable=
AddedNotification}}. It includes the memtable to allow consumers to know if=
 it comes from a flush.

bq. Also you should probably ubsubscribe from the tracker when the index is=
 dropped.

It is the {{SecondaryIndexManager}} who is subscribed to the tracker. It ma=
nages all the indexes on its column family store, so it shouldn't unsubscri=
be when one of its indexes is dropped, unless I'm missing something.

> Node failure during 2i update after streaming can have incomplete 2i when=
 restarted
> -------------------------------------------------------------------------=
----------
>
>                 Key: CASSANDRA-10130
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-1013=
0
>             Project: Cassandra
>          Issue Type: Bug
>          Components: Coordination
>            Reporter: Yuki Morishita
>            Assignee: Andr=C3=A9s de la Pe=C3=B1a
>            Priority: Minor
>
> Since MV/2i update happens after SSTables are received, node failure duri=
ng MV/2i update can leave received SSTables live when restarted while MV/2i=
 are partially up to date.
> We can add some kind of tracking mechanism to automatically rebuild at th=
e startup, or at least warn user when the node restarts.


--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org