jyothsnakonisa commented on code in PR #4557:
URL: https://github.com/apache/cassandra/pull/4557#discussion_r2729288910
##########
src/java/org/apache/cassandra/service/StartupChecks.java:
##########
@@ -150,12 +132,65 @@ public enum StartupCheckType
checkKernelParamsForAsyncProfiler,
new
DataResurrectionCheck());
+ public List<StartupCheck> getChecks()
+ {
+ return preFlightChecks;
Review Comment:
May be return immutable copy?
##########
src/java/org/apache/cassandra/service/StartupChecks.java:
##########
@@ -194,9 +229,15 @@ public void verify(StartupChecksOptions options) throws
StartupException
public static final StartupCheck checkKernelBug1057843 = new StartupCheck()
{
@Override
- public void execute(StartupChecksOptions startupChecksOptions) throws
StartupException
+ public String name()
+ {
+ return "kernel_bug_1057834";
Review Comment:
```suggestion
return "kernel_bug_1057843";
```
Looks like a mistake in the name, can you please fix it with whatever is
correct?
##########
src/java/org/apache/cassandra/config/StartupChecksConfiguration.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.config;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import org.apache.cassandra.exceptions.StartupException;
+import org.apache.cassandra.service.StartupCheck;
+import org.apache.cassandra.service.StartupChecks;
+
+import static java.lang.Boolean.FALSE;
+import static java.lang.Boolean.TRUE;
+
+public class StartupChecksConfiguration
+{
+ public static final String ENABLED_PROPERTY = "enabled";
+
+ private final Map<String, Map<String, Object>> options = new HashMap<>();
+ private final StartupChecks startupChecks;
+
+ public StartupChecksConfiguration(StartupChecks startupChecks, final
Map<String, Map<String, Object>> options)
+ {
+ this.options.putAll(options);
+ this.startupChecks = startupChecks;
+
+ apply(startupChecks, options);
+ }
+
+ @VisibleForTesting
+ public StartupCheck getCheck(String name)
+ {
+ return startupChecks.getCheck(name);
+ }
+
+ private StartupCheck getConfigurableCheck(String name)
+ {
+ StartupCheck check = startupChecks.getCheck(name);
+ if (check == null)
+ return null;
+
+ if (!check.isConfigurable())
+ return null;
+
+ return check;
+ }
+
+ public void set(final String name, final String key, final Object value)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ Map<String, Object> checkConfiguration = options.get(name);
+ if (checkConfiguration == null)
+ return;
+
+ checkConfiguration.put(key, value);
+ }
+
+ public void reset(String name)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ Map<String, Object> checkConfiguration = options.get(name);
+ if (checkConfiguration == null)
+ return;
+
+ boolean enabled = isEnabled(name);
+
+ checkConfiguration.clear();
+
+ checkConfiguration.put(ENABLED_PROPERTY, enabled);
+ }
+
+ public void enable(final String name)
+ {
+ set(name, ENABLED_PROPERTY, TRUE);
+ }
+
+ public void disable(final String name)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ set(name, ENABLED_PROPERTY, FALSE);
+ }
+
+ public boolean isEnabled(final String name)
+ {
+ Map<String, Object> config = getConfig(name);
+ if (config == null)
+ return false;
+
+ Object enabledBoolean = config.get(ENABLED_PROPERTY);
+ if (enabledBoolean == null)
+ return false;
+
+ return Boolean.parseBoolean(enabledBoolean.toString());
+ }
+
+ public boolean isDisabled(final String name)
+ {
+ return !isEnabled(name);
+ }
+
+ public Map<String, Object> getConfig(final String name)
+ {
+ return options.get(name);
+ }
+
+ private void apply(StartupChecks startupChecks, Map<String, Map<String,
Object>> options)
Review Comment:
`startupChecks` & `options` don't need to be passed as part of this method
as they are members of this class.
##########
src/java/org/apache/cassandra/config/StartupChecksConfiguration.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.config;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import org.apache.cassandra.exceptions.StartupException;
+import org.apache.cassandra.service.StartupCheck;
+import org.apache.cassandra.service.StartupChecks;
+
+import static java.lang.Boolean.FALSE;
+import static java.lang.Boolean.TRUE;
+
+public class StartupChecksConfiguration
+{
+ public static final String ENABLED_PROPERTY = "enabled";
+
+ private final Map<String, Map<String, Object>> options = new HashMap<>();
+ private final StartupChecks startupChecks;
+
+ public StartupChecksConfiguration(StartupChecks startupChecks, final
Map<String, Map<String, Object>> options)
+ {
+ this.options.putAll(options);
+ this.startupChecks = startupChecks;
+
+ apply(startupChecks, options);
+ }
+
+ @VisibleForTesting
+ public StartupCheck getCheck(String name)
+ {
+ return startupChecks.getCheck(name);
+ }
+
+ private StartupCheck getConfigurableCheck(String name)
+ {
+ StartupCheck check = startupChecks.getCheck(name);
+ if (check == null)
+ return null;
+
+ if (!check.isConfigurable())
+ return null;
+
+ return check;
+ }
+
+ public void set(final String name, final String key, final Object value)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ Map<String, Object> checkConfiguration = options.get(name);
+ if (checkConfiguration == null)
+ return;
+
+ checkConfiguration.put(key, value);
+ }
+
+ public void reset(String name)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ Map<String, Object> checkConfiguration = options.get(name);
+ if (checkConfiguration == null)
+ return;
+
+ boolean enabled = isEnabled(name);
+
+ checkConfiguration.clear();
+
+ checkConfiguration.put(ENABLED_PROPERTY, enabled);
Review Comment:
You are getting the enabled state, clearing the check configuration and
setting the enabled property again back to the previous state. You are not
actually resetting check's configuration
##########
src/java/org/apache/cassandra/config/StartupChecksConfiguration.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.config;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import org.apache.cassandra.exceptions.StartupException;
+import org.apache.cassandra.service.StartupCheck;
+import org.apache.cassandra.service.StartupChecks;
+
+import static java.lang.Boolean.FALSE;
+import static java.lang.Boolean.TRUE;
+
+public class StartupChecksConfiguration
+{
+ public static final String ENABLED_PROPERTY = "enabled";
+
+ private final Map<String, Map<String, Object>> options = new HashMap<>();
+ private final StartupChecks startupChecks;
+
+ public StartupChecksConfiguration(StartupChecks startupChecks, final
Map<String, Map<String, Object>> options)
+ {
+ this.options.putAll(options);
Review Comment:
May be make a defensive copy for added safety
##########
src/java/org/apache/cassandra/config/StartupChecksConfiguration.java:
##########
Review Comment:
There are inconsistencies in using final for method names in this class, can
you please make them consistent and remove final in method parameters.
##########
src/java/org/apache/cassandra/service/StartupCheck.java:
##########
Review Comment:
Few Interface method signatures are changed here, I am worried it might
break any existing implementations of this interface, do you think that we have
to handle backward compatibility with this change?
##########
src/java/org/apache/cassandra/service/StartupChecks.java:
##########
@@ -150,12 +132,65 @@ public enum StartupCheckType
checkKernelParamsForAsyncProfiler,
new
DataResurrectionCheck());
+ public List<StartupCheck> getChecks()
+ {
+ return preFlightChecks;
+ }
+
+ public StartupCheck getCheck(String name)
+ {
+ for (StartupCheck startupCheck : preFlightChecks)
+ {
+ if (startupCheck.name().equals(name))
+ return startupCheck;
+ }
+ return null;
+ }
+
public StartupChecks withDefaultTests()
{
preFlightChecks.addAll(DEFAULT_TESTS);
Review Comment:
Do we want to check for duplicate checks here or wherever checks are added
to `preFlightChecks`
##########
src/java/org/apache/cassandra/service/StartupChecks.java:
##########
@@ -150,12 +132,65 @@ public enum StartupCheckType
checkKernelParamsForAsyncProfiler,
new
DataResurrectionCheck());
+ public List<StartupCheck> getChecks()
+ {
+ return preFlightChecks;
+ }
+
+ public StartupCheck getCheck(String name)
+ {
+ for (StartupCheck startupCheck : preFlightChecks)
+ {
+ if (startupCheck.name().equals(name))
+ return startupCheck;
+ }
Review Comment:
Instead of iterating over entire list, can you make `preFlightChecks` a map
and just get the value based on the name?
##########
src/java/org/apache/cassandra/config/StartupChecksConfiguration.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.config;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import org.apache.cassandra.exceptions.StartupException;
+import org.apache.cassandra.service.StartupCheck;
+import org.apache.cassandra.service.StartupChecks;
+
+import static java.lang.Boolean.FALSE;
+import static java.lang.Boolean.TRUE;
+
+public class StartupChecksConfiguration
+{
+ public static final String ENABLED_PROPERTY = "enabled";
+
+ private final Map<String, Map<String, Object>> options = new HashMap<>();
+ private final StartupChecks startupChecks;
+
+ public StartupChecksConfiguration(StartupChecks startupChecks, final
Map<String, Map<String, Object>> options)
+ {
+ this.options.putAll(options);
+ this.startupChecks = startupChecks;
+
+ apply(startupChecks, options);
+ }
+
+ @VisibleForTesting
+ public StartupCheck getCheck(String name)
+ {
+ return startupChecks.getCheck(name);
+ }
+
+ private StartupCheck getConfigurableCheck(String name)
+ {
+ StartupCheck check = startupChecks.getCheck(name);
+ if (check == null)
+ return null;
+
+ if (!check.isConfigurable())
+ return null;
+
+ return check;
+ }
+
+ public void set(final String name, final String key, final Object value)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ Map<String, Object> checkConfiguration = options.get(name);
+ if (checkConfiguration == null)
+ return;
+
+ checkConfiguration.put(key, value);
+ }
+
+ public void reset(String name)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ Map<String, Object> checkConfiguration = options.get(name);
+ if (checkConfiguration == null)
+ return;
+
+ boolean enabled = isEnabled(name);
+
+ checkConfiguration.clear();
+
+ checkConfiguration.put(ENABLED_PROPERTY, enabled);
+ }
+
+ public void enable(final String name)
+ {
+ set(name, ENABLED_PROPERTY, TRUE);
+ }
+
+ public void disable(final String name)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ set(name, ENABLED_PROPERTY, FALSE);
Review Comment:
```suggestion
set(name, ENABLED_PROPERTY, FALSE);
```
For consistency with enable method
##########
src/java/org/apache/cassandra/service/StartupCheck.java:
##########
@@ -45,23 +53,36 @@ public interface StartupCheck
* @throws org.apache.cassandra.exceptions.StartupException if the test
determines
* that the environement or system is not in a safe state to startup
*/
- void execute(StartupChecksOptions startupChecksOptions) throws
StartupException;
+ void execute(StartupChecksConfiguration startupChecksOptions) throws
StartupException;
+
+ /**
+ * Tells whether a startup check can be configured, at the moment via
cassandra.yml.
+ *
+ * @return true if a startup check is configurable, false otherwise.
+ */
+ default boolean isConfigurable()
+ {
+ return false;
+ }
/**
+ * Tells if a specific (configurable) check is executed when it is not
specified in cassandra.yaml. By default,
+ * an implementation of a startup check is executed even if it is not
specified. For some checks, it might be
+ * preferential to not execute them when they are not explicity mentioned.
*
- * @return type of this startup check for configuration retrieval
+ * @return true if a check is executed regardless of it being specified in
cassandra.yaml
Review Comment:
Can you check this documentation statement once? as per my understanding
this should return true if the check is disabled by default.
##########
src/java/org/apache/cassandra/config/StartupChecksConfiguration.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.config;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import org.apache.cassandra.exceptions.StartupException;
+import org.apache.cassandra.service.StartupCheck;
+import org.apache.cassandra.service.StartupChecks;
+
+import static java.lang.Boolean.FALSE;
+import static java.lang.Boolean.TRUE;
+
+public class StartupChecksConfiguration
+{
+ public static final String ENABLED_PROPERTY = "enabled";
+
+ private final Map<String, Map<String, Object>> options = new HashMap<>();
+ private final StartupChecks startupChecks;
+
+ public StartupChecksConfiguration(StartupChecks startupChecks, final
Map<String, Map<String, Object>> options)
+ {
+ this.options.putAll(options);
+ this.startupChecks = startupChecks;
+
+ apply(startupChecks, options);
+ }
+
+ @VisibleForTesting
+ public StartupCheck getCheck(String name)
+ {
+ return startupChecks.getCheck(name);
+ }
+
+ private StartupCheck getConfigurableCheck(String name)
+ {
+ StartupCheck check = startupChecks.getCheck(name);
+ if (check == null)
+ return null;
+
+ if (!check.isConfigurable())
+ return null;
+
+ return check;
+ }
+
+ public void set(final String name, final String key, final Object value)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ Map<String, Object> checkConfiguration = options.get(name);
+ if (checkConfiguration == null)
+ return;
+
+ checkConfiguration.put(key, value);
+ }
+
+ public void reset(String name)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ Map<String, Object> checkConfiguration = options.get(name);
+ if (checkConfiguration == null)
+ return;
+
+ boolean enabled = isEnabled(name);
+
+ checkConfiguration.clear();
+
+ checkConfiguration.put(ENABLED_PROPERTY, enabled);
+ }
+
+ public void enable(final String name)
+ {
+ set(name, ENABLED_PROPERTY, TRUE);
+ }
+
+ public void disable(final String name)
+ {
+ StartupCheck check = getConfigurableCheck(name);
+ if (check == null)
+ return;
+
+ set(name, ENABLED_PROPERTY, FALSE);
+ }
+
+ public boolean isEnabled(final String name)
+ {
+ Map<String, Object> config = getConfig(name);
+ if (config == null)
+ return false;
+
+ Object enabledBoolean = config.get(ENABLED_PROPERTY);
+ if (enabledBoolean == null)
+ return false;
+
+ return Boolean.parseBoolean(enabledBoolean.toString());
+ }
+
+ public boolean isDisabled(final String name)
+ {
+ return !isEnabled(name);
+ }
+
+ public Map<String, Object> getConfig(final String name)
+ {
+ return options.get(name);
+ }
+
+ private void apply(StartupChecks startupChecks, Map<String, Map<String,
Object>> options)
+ {
+ List<String> notExistingCheckNames = new ArrayList<>();
+ List<String> notConfigurableCheckNames = new ArrayList<>();
+
+ for (Map.Entry<String, Map<String, Object>> userConfigEntry :
options.entrySet())
+ {
+ String key = userConfigEntry.getKey();
+ StartupCheck check = startupChecks.getCheck(key);
+ if (check == null)
+ notExistingCheckNames.add(key);
+ else if (!check.isConfigurable())
+ notConfigurableCheckNames.add(key);
+ }
+
+ if (!notExistingCheckNames.isEmpty())
+ throw new IllegalStateException("There are configuration entries
for startup checks which do not exist: " + notExistingCheckNames);
+ if (!notConfigurableCheckNames.isEmpty())
+ throw new IllegalStateException("There are configuration entries
for startup checks which are not configurable: " + notConfigurableCheckNames);
+
+ for (StartupCheck check : startupChecks.getChecks())
+ {
+ String startupCheckName = check.name();
+ Map<String, Object> configMap =
this.options.computeIfAbsent(startupCheckName, k -> new HashMap<>());
+ if (configMap.containsKey(ENABLED_PROPERTY))
+ configMap.putIfAbsent(ENABLED_PROPERTY, FALSE);
+ else if (check.isDisabledByDefault())
+ configMap.put(ENABLED_PROPERTY, FALSE);
+ else
+ configMap.put(ENABLED_PROPERTY, TRUE);
Review Comment:
```suggestion
if (!configMap.containsKey(ENABLED_PROPERTY))
{
if (check.isDisabledByDefault())
configMap.put(ENABLED_PROPERTY, FALSE);
else
configMap.put(ENABLED_PROPERTY, TRUE);
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
