You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "Runtian (via GitHub)" <gi...@apache.org> on 2023/03/02 03:59:59 UTC
[GitHub] [cassandra] Runtian opened a new pull request, #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Runtian opened a new pull request, #2189:
URL: https://github.com/apache/cassandra/pull/2189
To reproduce the issue, switch to disk_failure_policy die
add some code to trigger FSError.
trigger the FSError and Cassandra process will not stop.
With this fix, Cassandra process will stop.
```
Fix default file system error handler for disk_failure_policy die
patch by Runtian Liu; reviewed by TBD for CASSANDRA-18294
```
The [Cassandra Jira](https://issues.apache.org/jira/browse/CASSANDRA-18294)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122756049
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
Is not it true that when policy is `die` and we reach this method, instead of killing the jvm, you will only stop transports? There is this in `cassandra.yaml`:
````
# die
# shut down gossip and client transports and kill the JVM for any fs errors or
# single-sstable errors, so the node can be replaced.
````
As I read it, we should just kill whole JVM, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1123416456
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
@Runtian can you actually explain where what does this change do? Because to my understanding it will just stop transports? But the objective is to kill the whole jvm? What are we stopping the transports for when we kill the jvm anyway?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1123416456
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
@Runtian can you actually explain what this change does? Because to my understanding it will just stop transports? But the objective is to kill the whole jvm? What are we stopping the transports for when we kill the jvm anyway?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1123416456
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
@Runtian can you actually explain what this change does? Because to my understanding it will just stop transports? But the objective is to kill the whole jvm? What are we stopping the transports for when we kill the jvm anyway?
It is here to not reach "default" so IllegalStateException is not thrown?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122728821
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
+{
+ FSErrorHandler handler = new DefaultFSErrorHandler();
+ Config.DiskFailurePolicy oldDiskPolicy;
+
+ @BeforeClass
+ public static void defineSchema() throws ConfigurationException
+ {
+ SchemaLoader.prepareServer();
+ CassandraDaemon daemon = new CassandraDaemon();
+ daemon.completeSetup(); //startup must be completed, otherwise FS error will kill JVM regardless of failure policy
+ StorageService.instance.registerDaemon(daemon);
+ StorageService.instance.initServer();
+ }
+
+ @AfterClass
+ public static void shutdown()
+ {
+ StorageService.instance.stopClient();
+ }
+
+ @Before
+ public void setup()
+ {
+ StorageService.instance.startGossiping();
+ assertTrue(Gossiper.instance.isEnabled());
+ oldDiskPolicy = DatabaseDescriptor.getDiskFailurePolicy();
+ }
+
+ @After
+ public void teardown()
+ {
+ DatabaseDescriptor.setDiskFailurePolicy(oldDiskPolicy);
+ }
+
+ @Test
+ public void testFSErrorPolicyIgnore()
+ {
+ assertEquals(DatabaseDescriptor.getDiskFailurePolicy(), Config.DiskFailurePolicy.ignore);
+ handler.handleFSError(new FSReadError(new IOException(), "blah"));
+ // error ignored, gossip not disabled
+ assertTrue(Gossiper.instance.isEnabled());
+ }
+
+ @Test
+ public void testFSErrorPolicyStop()
+ {
Review Comment:
Yes, I agree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1128056141
##########
test/unit/org/apache/cassandra/utils/KillerForTests.java:
##########
@@ -29,6 +29,10 @@ public class KillerForTests extends JVMStabilityInspector.Killer
@Override
protected void killCurrentJVM(Throwable t, boolean quiet)
{
+ if (killed) {
Review Comment:
braces (you could go without braces as well as there is just `return` here)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1123066359
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
ah right ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Runtian commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "Runtian (via GitHub)" <gi...@apache.org>.
Runtian commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122623493
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
+{
+ FSErrorHandler handler = new DefaultFSErrorHandler();
+ Config.DiskFailurePolicy oldDiskPolicy;
+
+ @BeforeClass
+ public static void defineSchema() throws ConfigurationException
+ {
+ SchemaLoader.prepareServer();
+ CassandraDaemon daemon = new CassandraDaemon();
+ daemon.completeSetup(); //startup must be completed, otherwise FS error will kill JVM regardless of failure policy
+ StorageService.instance.registerDaemon(daemon);
+ StorageService.instance.initServer();
+ }
+
+ @AfterClass
+ public static void shutdown()
+ {
+ StorageService.instance.stopClient();
+ }
+
+ @Before
+ public void setup()
+ {
+ StorageService.instance.startGossiping();
+ assertTrue(Gossiper.instance.isEnabled());
+ oldDiskPolicy = DatabaseDescriptor.getDiskFailurePolicy();
+ }
+
+ @After
+ public void teardown()
+ {
+ DatabaseDescriptor.setDiskFailurePolicy(oldDiskPolicy);
+ }
+
+ @Test
+ public void testFSErrorPolicyIgnore()
+ {
+ assertEquals(DatabaseDescriptor.getDiskFailurePolicy(), Config.DiskFailurePolicy.ignore);
+ handler.handleFSError(new FSReadError(new IOException(), "blah"));
Review Comment:
just use the same exception from JVMStabilityInspectorTest.java
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
Review Comment:
This handler should also stop rpc server as well as native transport if they are running.
But it is hard to start the mentioned services in unit tests.
CommitLogFailurePolicyTest.testCommitFailurePolicy_stop is also only verifying gossip.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic closed pull request #2189: CASSANDRA-18294 3.0 Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic closed pull request #2189: CASSANDRA-18294 3.0 Fix disk_failure_policy die to stop gossip, native transport and JVM
URL: https://github.com/apache/cassandra/pull/2189
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122604634
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
Review Comment:
remove ununsed import
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122729190
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
+{
+ FSErrorHandler handler = new DefaultFSErrorHandler();
+ Config.DiskFailurePolicy oldDiskPolicy;
+
+ @BeforeClass
+ public static void defineSchema() throws ConfigurationException
+ {
+ SchemaLoader.prepareServer();
+ CassandraDaemon daemon = new CassandraDaemon();
+ daemon.completeSetup(); //startup must be completed, otherwise FS error will kill JVM regardless of failure policy
+ StorageService.instance.registerDaemon(daemon);
+ StorageService.instance.initServer();
+ }
+
+ @AfterClass
+ public static void shutdown()
+ {
+ StorageService.instance.stopClient();
+ }
+
+ @Before
+ public void setup()
+ {
+ StorageService.instance.startGossiping();
+ assertTrue(Gossiper.instance.isEnabled());
+ oldDiskPolicy = DatabaseDescriptor.getDiskFailurePolicy();
+ }
+
+ @After
+ public void teardown()
+ {
+ DatabaseDescriptor.setDiskFailurePolicy(oldDiskPolicy);
+ }
+
+ @Test
+ public void testFSErrorPolicyIgnore()
+ {
+ assertEquals(DatabaseDescriptor.getDiskFailurePolicy(), Config.DiskFailurePolicy.ignore);
+ handler.handleFSError(new FSReadError(new IOException(), "blah"));
Review Comment:
put there `exception message` instead of `blah`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Runtian commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "Runtian (via GitHub)" <gi...@apache.org>.
Runtian commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1123423301
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
Right, it is here to not reach the default so IllegalStateException will not be thrown.
We can also just add die with ignore to do nothing, but I think it is better to add die with stop and stop_paranoid so it tries to stop the transport and gossip etc. Also added the similar change to `handleCorruptSSTable`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1128055892
##########
test/unit/org/apache/cassandra/service/DiskFailurePolicyTest.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.Config.DiskFailurePolicy;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+import org.apache.cassandra.io.util.FileUtils;
+import org.apache.cassandra.utils.JVMStabilityInspector;
+import org.apache.cassandra.utils.KillerForTests;
+
+@RunWith(Parameterized.class)
+public class DiskFailurePolicyTest
+{
+ DiskFailurePolicy originalDiskFailurePolicy;
+ JVMStabilityInspector.Killer originalKiller;
+ KillerForTests killerForTests;
+ DiskFailurePolicy testPolicy;
+ boolean isStartUpInProgress;
+ Throwable t;
+ boolean expectGossipRunning;
+ boolean expectJVMKilled;
+ boolean expectJVMKilledQuiet;
+
+
+ @BeforeClass
+ public static void defineSchema() throws ConfigurationException
+ {
+ SchemaLoader.prepareServer();
+ StorageService.instance.initServer();
+ FileUtils.setFSErrorHandler(new DefaultFSErrorHandler());
+ }
+
+ public DiskFailurePolicyTest(DiskFailurePolicy testPolicy, boolean isStartUpInProgress, Throwable t,
+ boolean expectGossipRunning, boolean jvmKilled, boolean jvmKilledQuiet) {
+ this.testPolicy = testPolicy;
+ this.isStartUpInProgress = isStartUpInProgress;
+ this.t = t;
+ this.expectGossipRunning = expectGossipRunning;
+ this.expectJVMKilled = jvmKilled;
+ this.expectJVMKilledQuiet = jvmKilledQuiet;
+ }
+
+ @Parameterized.Parameters
+ public static Collection<Object[]> generateData()
+ {
+ return Arrays.asList(new Object[][]{
+ { Config.DiskFailurePolicy.die, true, new FSReadError(new IOException(), "blah"), false, true, true},
+ { DiskFailurePolicy.ignore, true, new FSReadError(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop, true, new FSReadError(new IOException(), "blah"), false, true, true},
+ { DiskFailurePolicy.stop_paranoid, true, new FSReadError(new IOException(), "blah"), false, true, true},
+ { Config.DiskFailurePolicy.die, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true},
+ { DiskFailurePolicy.ignore, true, new CorruptSSTableException(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true},
+ { DiskFailurePolicy.stop_paranoid, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true},
+ { Config.DiskFailurePolicy.die, false, new FSReadError(new IOException(), "blah"), false, true, false},
+ { DiskFailurePolicy.ignore, false, new FSReadError(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop, false, new FSReadError(new IOException(), "blah"), false, false, false},
+ { DiskFailurePolicy.stop_paranoid, false, new FSReadError(new IOException(), "blah"), false, false, false},
+ { Config.DiskFailurePolicy.die, false, new CorruptSSTableException(new IOException(), "blah"), false, true, false},
+ { DiskFailurePolicy.ignore, false, new CorruptSSTableException(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop, false, new CorruptSSTableException(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop_paranoid, false, new CorruptSSTableException(new IOException(), "blah"), false, false, false}
+ }
+ );
+ }
+
+ @Before
+ public void setup()
+ {
+ CassandraDaemon daemon = new CassandraDaemon();
+ if (!isStartUpInProgress)
+ daemon.completeSetup(); //mark startup completed
+ StorageService.instance.registerDaemon(daemon);
+ killerForTests = new KillerForTests();
+ originalKiller = JVMStabilityInspector.replaceKiller(killerForTests);
+ originalDiskFailurePolicy = DatabaseDescriptor.getDiskFailurePolicy();
+ StorageService.instance.startGossiping();
+ Assert.assertTrue(Gossiper.instance.isEnabled());
+ }
+
+ @After
+ public void teardown() {
+ JVMStabilityInspector.replaceKiller(originalKiller);
+ DatabaseDescriptor.setDiskFailurePolicy(originalDiskFailurePolicy);
+ }
+
+ @Test
+ public void testPolicies() {
+ DatabaseDescriptor.setDiskFailurePolicy(testPolicy);
+ JVMStabilityInspector.inspectThrowable(t);
+ Assert.assertEquals(expectJVMKilled, killerForTests.wasKilled());
+ Assert.assertEquals(expectJVMKilledQuiet, killerForTests.wasKilledQuietly());
+ if (!expectJVMKilled) {
+ // only verify gossip if JVM is not killed
+ Assert.assertEquals(expectGossipRunning, Gossiper.instance.isEnabled());
+ }
+
Review Comment:
nit: empty line
##########
test/unit/org/apache/cassandra/utils/KillerForTests.java:
##########
@@ -29,6 +29,10 @@ public class KillerForTests extends JVMStabilityInspector.Killer
@Override
protected void killCurrentJVM(Throwable t, boolean quiet)
{
+ if (killed) {
Review Comment:
braces
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1128051397
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
Review Comment:
@Runtian what is hard about starting it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: CASSANDRA-18294 3.0 Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1128051397
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
Review Comment:
@Runtian what is hard about starting it?
You know what just leave it as it is ... this is enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122613958
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
+{
+ FSErrorHandler handler = new DefaultFSErrorHandler();
Review Comment:
can they be private ?
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
+{
+ FSErrorHandler handler = new DefaultFSErrorHandler();
+ Config.DiskFailurePolicy oldDiskPolicy;
+
+ @BeforeClass
+ public static void defineSchema() throws ConfigurationException
+ {
+ SchemaLoader.prepareServer();
+ CassandraDaemon daemon = new CassandraDaemon();
+ daemon.completeSetup(); //startup must be completed, otherwise FS error will kill JVM regardless of failure policy
+ StorageService.instance.registerDaemon(daemon);
+ StorageService.instance.initServer();
+ }
+
+ @AfterClass
+ public static void shutdown()
+ {
+ StorageService.instance.stopClient();
+ }
+
+ @Before
+ public void setup()
+ {
+ StorageService.instance.startGossiping();
+ assertTrue(Gossiper.instance.isEnabled());
+ oldDiskPolicy = DatabaseDescriptor.getDiskFailurePolicy();
+ }
+
+ @After
+ public void teardown()
+ {
+ DatabaseDescriptor.setDiskFailurePolicy(oldDiskPolicy);
+ }
+
+ @Test
+ public void testFSErrorPolicyIgnore()
+ {
+ assertEquals(DatabaseDescriptor.getDiskFailurePolicy(), Config.DiskFailurePolicy.ignore);
+ handler.handleFSError(new FSReadError(new IOException(), "blah"));
+ // error ignored, gossip not disabled
+ assertTrue(Gossiper.instance.isEnabled());
+ }
+
+ @Test
+ public void testFSErrorPolicyStop()
+ {
Review Comment:
as all your test case's code logic are same with different DiskFailurePolicy
I think it is better to use one test case with different Parameterized.Parameters [stop, die, ignore, stop_paranoid]
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
+{
+ FSErrorHandler handler = new DefaultFSErrorHandler();
+ Config.DiskFailurePolicy oldDiskPolicy;
+
+ @BeforeClass
+ public static void defineSchema() throws ConfigurationException
+ {
+ SchemaLoader.prepareServer();
+ CassandraDaemon daemon = new CassandraDaemon();
+ daemon.completeSetup(); //startup must be completed, otherwise FS error will kill JVM regardless of failure policy
+ StorageService.instance.registerDaemon(daemon);
+ StorageService.instance.initServer();
+ }
+
+ @AfterClass
+ public static void shutdown()
+ {
+ StorageService.instance.stopClient();
+ }
+
+ @Before
+ public void setup()
+ {
+ StorageService.instance.startGossiping();
+ assertTrue(Gossiper.instance.isEnabled());
+ oldDiskPolicy = DatabaseDescriptor.getDiskFailurePolicy();
+ }
+
+ @After
+ public void teardown()
+ {
+ DatabaseDescriptor.setDiskFailurePolicy(oldDiskPolicy);
+ }
+
+ @Test
+ public void testFSErrorPolicyIgnore()
+ {
+ assertEquals(DatabaseDescriptor.getDiskFailurePolicy(), Config.DiskFailurePolicy.ignore);
+ handler.handleFSError(new FSReadError(new IOException(), "blah"));
Review Comment:
what does blah mean ?
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
Review Comment:
it is better to rename the class name to DefaultFSErrorHandlerForGossipTest
if this test is only for gossip's status
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122757781
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
Also, do not we miss `die` policy in `handleCorruptSSTable` method in this class? There is only `stop_paranoid`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122729190
##########
test/unit/org/apache/cassandra/service/DefaultFSErrorHandlerTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSErrorHandler;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class DefaultFSErrorHandlerTest
+{
+ FSErrorHandler handler = new DefaultFSErrorHandler();
+ Config.DiskFailurePolicy oldDiskPolicy;
+
+ @BeforeClass
+ public static void defineSchema() throws ConfigurationException
+ {
+ SchemaLoader.prepareServer();
+ CassandraDaemon daemon = new CassandraDaemon();
+ daemon.completeSetup(); //startup must be completed, otherwise FS error will kill JVM regardless of failure policy
+ StorageService.instance.registerDaemon(daemon);
+ StorageService.instance.initServer();
+ }
+
+ @AfterClass
+ public static void shutdown()
+ {
+ StorageService.instance.stopClient();
+ }
+
+ @Before
+ public void setup()
+ {
+ StorageService.instance.startGossiping();
+ assertTrue(Gossiper.instance.isEnabled());
+ oldDiskPolicy = DatabaseDescriptor.getDiskFailurePolicy();
+ }
+
+ @After
+ public void teardown()
+ {
+ DatabaseDescriptor.setDiskFailurePolicy(oldDiskPolicy);
+ }
+
+ @Test
+ public void testFSErrorPolicyIgnore()
+ {
+ assertEquals(DatabaseDescriptor.getDiskFailurePolicy(), Config.DiskFailurePolicy.ignore);
+ handler.handleFSError(new FSReadError(new IOException(), "blah"));
Review Comment:
Looking into `FSReadError` constructors, the second parameter is meant to represent "path" to a file which caused the exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Maxwell-Guo commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "Maxwell-Guo (via GitHub)" <gi...@apache.org>.
Maxwell-Guo commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122779198
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/utils/JVMStabilityInspector.java#L142
@smiklosovic I think at this step we will kill jvm when policy is die with FSerror or CorruptSSTableException
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1122737270
##########
src/java/org/apache/cassandra/service/DefaultFSErrorHandler.java:
##########
@@ -60,6 +60,7 @@ public void handleFSError(FSError e)
switch (DatabaseDescriptor.getDiskFailurePolicy())
{
+ case die:
Review Comment:
the order of these enums in this switch is different from the order in `handleStartupFSError`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #2189: Fix disk_failure_policy die to stop gossip, native transport and JVM
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #2189:
URL: https://github.com/apache/cassandra/pull/2189#discussion_r1128055121
##########
test/unit/org/apache/cassandra/service/DiskFailurePolicyTest.java:
##########
@@ -0,0 +1,133 @@
+/*
+ * 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.service;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.apache.cassandra.SchemaLoader;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.config.Config.DiskFailurePolicy;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.exceptions.ConfigurationException;
+import org.apache.cassandra.gms.Gossiper;
+import org.apache.cassandra.io.FSReadError;
+import org.apache.cassandra.io.sstable.CorruptSSTableException;
+import org.apache.cassandra.io.util.FileUtils;
+import org.apache.cassandra.utils.JVMStabilityInspector;
+import org.apache.cassandra.utils.KillerForTests;
+
+@RunWith(Parameterized.class)
+public class DiskFailurePolicyTest
+{
+ DiskFailurePolicy originalDiskFailurePolicy;
+ JVMStabilityInspector.Killer originalKiller;
+ KillerForTests killerForTests;
+ DiskFailurePolicy testPolicy;
+ boolean isStartUpInProgress;
+ Throwable t;
+ boolean expectGossipRunning;
+ boolean expectJVMKilled;
+ boolean expectJVMKilledQuiet;
+
+
+ @BeforeClass
+ public static void defineSchema() throws ConfigurationException
+ {
+ SchemaLoader.prepareServer();
+ StorageService.instance.initServer();
+ FileUtils.setFSErrorHandler(new DefaultFSErrorHandler());
+ }
+
+ public DiskFailurePolicyTest(DiskFailurePolicy testPolicy, boolean isStartUpInProgress, Throwable t,
+ boolean expectGossipRunning, boolean jvmKilled, boolean jvmKilledQuiet) {
+ this.testPolicy = testPolicy;
+ this.isStartUpInProgress = isStartUpInProgress;
+ this.t = t;
+ this.expectGossipRunning = expectGossipRunning;
+ this.expectJVMKilled = jvmKilled;
+ this.expectJVMKilledQuiet = jvmKilledQuiet;
+ }
+
+ @Parameterized.Parameters
+ public static Collection<Object[]> generateData()
+ {
+ return Arrays.asList(new Object[][]{
+ { Config.DiskFailurePolicy.die, true, new FSReadError(new IOException(), "blah"), false, true, true},
+ { DiskFailurePolicy.ignore, true, new FSReadError(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop, true, new FSReadError(new IOException(), "blah"), false, true, true},
+ { DiskFailurePolicy.stop_paranoid, true, new FSReadError(new IOException(), "blah"), false, true, true},
+ { Config.DiskFailurePolicy.die, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true},
+ { DiskFailurePolicy.ignore, true, new CorruptSSTableException(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true},
+ { DiskFailurePolicy.stop_paranoid, true, new CorruptSSTableException(new IOException(), "blah"), false, true, true},
+ { Config.DiskFailurePolicy.die, false, new FSReadError(new IOException(), "blah"), false, true, false},
+ { DiskFailurePolicy.ignore, false, new FSReadError(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop, false, new FSReadError(new IOException(), "blah"), false, false, false},
+ { DiskFailurePolicy.stop_paranoid, false, new FSReadError(new IOException(), "blah"), false, false, false},
+ { Config.DiskFailurePolicy.die, false, new CorruptSSTableException(new IOException(), "blah"), false, true, false},
+ { DiskFailurePolicy.ignore, false, new CorruptSSTableException(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop, false, new CorruptSSTableException(new IOException(), "blah"), true, false, false},
+ { DiskFailurePolicy.stop_paranoid, false, new CorruptSSTableException(new IOException(), "blah"), false, false, false}
+ }
+ );
+ }
+
+ @Before
+ public void setup()
+ {
+ CassandraDaemon daemon = new CassandraDaemon();
+ if (!isStartUpInProgress)
+ daemon.completeSetup(); //mark startup completed
+ StorageService.instance.registerDaemon(daemon);
+ killerForTests = new KillerForTests();
+ originalKiller = JVMStabilityInspector.replaceKiller(killerForTests);
+ originalDiskFailurePolicy = DatabaseDescriptor.getDiskFailurePolicy();
+ StorageService.instance.startGossiping();
+ Assert.assertTrue(Gossiper.instance.isEnabled());
+ }
+
+ @After
+ public void teardown() {
Review Comment:
braces should be on the new line, next test method should be fixed too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org