cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrés de la Peña (JIRA) <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-10130) Node failure during 2i update after streaming can have incomplete 2i when restarted
Date Thu, 15 Jun 2017 16:18:00 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-10130?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16050702#comment-16050702
] 

Andrés de la Peña commented on CASSANDRA-10130:
-----------------------------------------------

Here is another version of the patch:

||[trunk|https://github.com/apache/cassandra/compare/trunk...adelapena:10130-trunk]|[utests|http://cassci.datastax.com/view/Dev/view/adelapena/job/adelapena-10130-trunk-testall/]|[dtests|http://cassci.datastax.com/view/Dev/view/adelapena/job/adelapena-10130-trunk-dtest/]|

bq. I mistakenly removed {{queryableIndexes]] in my refactor (as noted); [this|https://github.com/adelapena/cassandra/commit/c5604ffd5ae36cc34d13af6c4c4eadba98458fa3]
commit added it back, but it's unfortunately not enough, as if the initialization task fails,
the index will never be made queryable: we need to fix this, and we need a test for such failure
case.

[Here|https://github.com/adelapena/cassandra/commit/2994aece1e936b81ad507f2e9baf0092e79edce0]
a successful full rebuild adds the index to the set of queryable indexes. I have also added
[some tests|https://github.com/adelapena/cassandra/blob/2994aece1e936b81ad507f2e9baf0092e79edce0/test/unit/org/apache/cassandra/index/SecondaryIndexManagerTest.java#L386-L456]
to check the behaviour. Note that a partial build doesn't set the index as queryable.

bq. Nit/OCD: I would change the [rebuildFromSSTablesBlocking|https://github.com/adelapena/cassandra/commit/a945358a36f97632e94e9103650d72c04735354d#diff-3f2c8994c4ff8748c3faf7e70958520dR306]
signature to have the {{sstables}} first, consistently with {{buildIndexesBlocking}}.

Initially done [here|https://github.com/adelapena/cassandra/commit/7dfc959703231c9f90f53a3b474cf7f66b240213],
but see three quotes below.

bq. In {{buildIndexesBlocking}}, any exceptions thrown by {{flushIndexesBlocking}} in the
{{finally}} block will hide previous exceptions: we should accumulate them.

Done [here|https://github.com/adelapena/cassandra/commit/fbce3657132a8b87d2ff2446504634f5efdf2b59].

bq. Nit/OCD: In buildIndexesBlocking, we should put an additional comment before invoking
flushIndexesBlocking.

Done [here|https://github.com/adelapena/cassandra/commit/b7bf81acd66c3bdac66ba86117e07cac251ca312].

bq. If {{rebuildFromSSTablesBlocking}} is not used elsewhere, I would strongly recommend to
make it private/protected and move its comment to {{rebuildIndexesBlocking}}: the [general
rule|https://image.slidesharecdn.com/effectivejava-2ndedition-chapter4-151203200429-lva1-app6891/95/effective-java-chapter-4-classes-and-interfaces-4-638.jpg?cb=1449173201]
is methods (as well as attributes) should have the most restricted visibility allowed by working
code.

Actually {{rebuildFromSSTablesBlocking}} is only called by full rebuilds, so we can eve get
rid of this method and move its content and doc to {{rebuildIndexesBlocking}}, as it is done
[here|https://github.com/adelapena/cassandra/commit/cdd32e54f86bd831faa4463489bad153520e2754].
Tests still have access to partial builds through [{{handleNotification}}|https://github.com/adelapena/cassandra/blob/fbce3657132a8b87d2ff2446504634f5efdf2b59/test/unit/org/apache/cassandra/index/SecondaryIndexManagerTest.java#L115].

bq. Regarding the {{test_standalone_scrub failure}}, I believe it's due to the fact we only
manage counters in our marking methods when {{DatabaseDescriptor.isDaemonInitialized()}},
which is obviously not the case with the scrubber; if so, it's probably an easy fix.

Right, it's fixed [here|https://github.com/adelapena/cassandra/commit/a2744560b2d3262b1872c5f989d1baee645b7c5c].

Apart form this, [here|https://github.com/adelapena/cassandra/commit/70b477bc4c83613474756d39ac36c1301c4a54b2]
I have restored {{CassandraIndexTest#indexCorrectlyMarkedAsBuildAndRemoved}} to its initial
shape because all the added cases are (better) covered by {{SecondaryIndexManagerTest}}. The
reason to preserve the test is that it uses a real index implementation instead of a mocked
one, which can be useful although the difficulties to simulate failures, blocking, etc.

I have also removed some unused imports.

> Node failure during 2i update after streaming can have incomplete 2i when restarted
> -----------------------------------------------------------------------------------
>
>                 Key: CASSANDRA-10130
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-10130
>             Project: Cassandra
>          Issue Type: Bug
>          Components: Coordination
>            Reporter: Yuki Morishita
>            Assignee: Andrés de la Peña
>            Priority: Minor
>
> Since MV/2i update happens after SSTables are received, node failure during 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 the startup, or
at least warn user when the node restarts.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

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


Mime
View raw message