Mmuzaf commented on code in PR #2046:
URL: https://github.com/apache/cassandra/pull/2046#discussion_r1171684701
##########
src/java/org/apache/cassandra/config/CassandraRelevantProperties.java:
##########
@@ -352,14 +410,166 @@
* This is an optimization used in unit tests becuase we never restart a
node there. The only node is stopoped
* when the JVM terminates. Therefore, we can use such optimization and
not wait unnecessarily. */
NON_GRACEFUL_SHUTDOWN("cassandra.test.messagingService.nonGracefulShutdown"),
+ CHRONICLE_ANNOUNCER_DISABLE("chronicle.announcer.disable"),
+
COMMITLOG_ALLOW_IGNORE_SYNC_CRC("cassandra.commitlog.allow_ignore_sync_crc"),
+ COMMITLOG_IGNORE_REPLAY_ERRORS("cassandra.commitlog.ignorereplayerrors"),
+
COMMITLOG_MAX_OUTSTANDING_REPLAY_BYTES("cassandra.commitlog_max_outstanding_replay_bytes",
String.valueOf(1024 * 1024 * 64)),
+
COMMITLOG_MAX_OUTSTANDING_REPLAY_COUNT("cassandra.commitlog_max_outstanding_replay_count",
"1024"),
+ COMMITLOG_STOP_ON_ERRORS("cassandra.commitlog.stop_on_errors"),
+ CONFIG_LOADER("cassandra.config.loader"),
+ CONSISTENT_RANGE_MOVEMENT("cassandra.consistent.rangemovement", "true"),
+
CONSISTENT_SIMULTANEOUS_MOVES_ALLOW("cassandra.consistent.simultaneousmoves.allow",
"false"),
+
CUSTOM_GUARDRAILS_CONFIG_PROVIDER_CLASS("cassandra.custom_guardrails_config_provider_class"),
+ CUSTOM_QUERY_HANDLER_CLASS("cassandra.custom_query_handler_class"),
+ CUSTOM_TRACING_CLASS("cassandra.custom_tracing_class"),
+
DATA_OUTPUT_STREAM_PLUS_TEMP_BUFFER_SIZE("cassandra.data_output_stream_plus_temp_buffer_size",
"8192"),
+
DECAYING_ESTIMATED_HISTOGRAM_RESERVOIR_STRIPE_COUNT("cassandra.dehr_stripe_count",
"2"),
+
DIAGNOSTIC_SNAPSHOT_INTERVAL_NANOS("cassandra.diagnostic_snapshot_interval_nanos",
"60000000000"),
+
DISABLE_AUTH_CACHES_REMOTE_CONFIGURATION("cassandra.disable_auth_caches_remote_configuration"),
+ DISABLE_PAXOS_AUTO_REPAIRS("cassandra.disable_paxos_auto_repairs"),
+ DISABLE_PAXOS_STATE_FLUSH("cassandra.disable_paxos_state_flush"),
+ DISABLE_STCS_IN_L0("cassandra.disable_stcs_in_l0"),
+ DISABLE_TCACTIVE_OPENSSL("cassandra.disable_tcactive_openssl"),
+ DOB_DOUBLING_THRESHOLD_MB("cassandra.DOB_DOUBLING_THRESHOLD_MB", "64"),
+ DOB_MAX_RECYCLE_BYTES("cassandra.dob_max_recycle_bytes",
String.valueOf(1024 * 1024)),
+
DROP_OVERSIZED_READ_REPAIR_MUTATIONS("cassandra.drop_oversized_readrepair_mutations"),
+ DTEST_API_LOG_TOPOLOGY("cassandra.dtest.api.log.topology"),
+ ENABLE_DC_LOCAL_COMMIT("cassandra.enable_dc_local_commit", "true"),
+
EXPIRATION_DATE_OVERFLOW_POLICY("cassandra.expiration_date_overflow_policy"),
+
EXPIRATION_OVERFLOW_WARNING_INTERVAL_MINUTES("cassandra.expiration_overflow_warning_interval_minutes",
"5"),
+ FD_INITIAL_VALUE_MS("cassandra.fd_initial_value_ms"),
+ FD_MAX_INTERVAL_MS("cassandra.fd_max_interval_ms"),
+ FILE_CACHE_ENABLED("cassandra.file_cache_enabled"),
+
FORCE_DEFAULT_INDEXING_PAGE_SIZE("cassandra.force_default_indexing_page_size"),
+ FORCE_PAXOS_STATE_REBUILD("cassandra.force_paxos_state_rebuild"),
+ GIT_SHA("cassandra.gitSHA"),
+
GOSSIP_DISABLE_THREAD_VALIDATION("cassandra.gossip.disable_thread_validation",
"false"),
+ IGNORE_CORRUPTED_SCHEMA_TABLES("cassandra.ignore_corrupted_schema_tables",
"false"),
Review Comment:
@jacek-lewandowski
If we're talking about the API for Cassandra's properties, it's not enough
to just focus on the `CassandraRelevantProperties` class itself, and I'd say we
should look at the scope a bit wider and put all configuration properties
(configuration, system properties or whatever) under the same kind of
interfaces and API. It was partially mentioned in Benedict's comment
[here](https://issues.apache.org/jira/browse/CASSANDRA-15254?focusedCommentId=17451067&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17451067)
and I have also been confronted with a lack of API to access config properties
to expose them through the settings virtual table -
[here](https://github.com/apache/cassandra/pull/2133/files#diff-931d44ec5e102506bca140864bb516f55c00028a95392b7eee3c647f22293339R56).
I'm working on the API to access the configuration properties for virtual
tables (the `getBoolean`, `getInt` methods for primitive values are omitted in
the draft implementation) - the `Registry` prototyped
[here](https://github.com/apache/cassandra/pull/2133/files#diff-58bd5ac9ec3b4e19a95f2669860fa72d69f4290354376662e8d8772f64af05feR37),
and I think it will be good to reuse the API for all sorts of configuration
property stores in Cassandra, and also align the property naming in the end.
You've also partially mentioned that we should have the same interface for
CassandraRelevantProperties and CassandraRelevantEnv, which I agree with.
So I think we are talking about the same thing from slightly different
angles. We should have the `Registry` - a common API interface for accessing
properties with:
- `ConfiguraitonRegistry` - to access the properties stored in the Config
class;
- `CassandraRelevantProperties` - to access the Cassandra's system
properties;
- `CassandraRelevantEnv` - to access the Cassandra's environment variables;
Having everything above under the common interface allows us to build a
configuration model tree for all of the ways Cassandra is configured.
## The API in Design Details
I think we should have a discussion on the dev-list for sure, but as a
draft, I can share the following:
1. I think we should allow default values for Cassandra's system properties
and forbid the same for non-cassandra properties as you mentioned it (e.g. for
those that are not followed with `cassandra.` forbid defaults). We have
defaults for some of the properties in the Config class, so it sounds logical
to keep the same behaviour for the `CassandraRelevantProperties`.
2. `getBooleanOpt()` I'm not against this method at all, but the `Optional`
is not enough for all the cases we have. We should allow internally in the
`map` method to handle not only `unchecked` exceptions, but also checked ones
that may affect the final implementation. The example is
[here](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/config/DatabaseDescriptor.java#L1839).
3. If we go with triplets like `getBoolean()`, `getBoolean(boolean
defaultValue)`, `getBooleanOpt()`, we will have them for every primitive type
used by `CassandraRelevantProperties` variables. So the final API will have the
3 times multiplier on the total methods and considering the fact that
Cassandra's `getBooleanOpt()` should also be aware of checked exceptions, the
complexity may become even greater.
My guess is that we could have an intermediate interface for getting values.
I have the following `Registry`
[here](https://github.com/apache/cassandra/pull/2133/files#diff-58bd5ac9ec3b4e19a95f2669860fa72d69f4290354376662e8d8772f64af05feR37),
please note it doesn't take into account the `Optional` approach you
suggested, but if we want to go with `Optional` we could have something similar:
```
public interface Registry
{
ConfigValue get(String key);
}
```
```
public interface ConfigValue
{
Object get();
String convertToString();
boolean getBoolean();
int getInt();
long getLong();
Class<?> type();
ConfigValue map(CheckedFunction<? super ConfigValue, ? extends
ConfigValue> mapper);
boolean ifPresent();
}
```
4. If we go with the changes above I think we should add type for each
property in the `ConfiguraitonRegistry` e.g.
`ACQUIRE_RETRY_SECONDS("cassandra.acquire_retry_seconds", Integer.class, "60")`
## Current changes
Going back to the scope of these changes, I can propose the following plan:
- forbid the defaults for non-cassandra system properties in this PR;
- `getBoolean()` should throw an exception if the value does not exist;
- set system property defaults or use `getBoolean(bolean
overrideDefaultValue)` where it's possible;
- The combination of `ifPresent()`, `getBoolean()`, `getBoolean(boolean
overrideDefaultValue)` covers all the cases we have now;
- Create an API design issue for all Config, CassandraRelevantProperties,
CassandraRelevantEnv classes or do the design part under
[CASSANDRA-15254](https://issues.apache.org/jira/browse/CASSANDRA-15254) as the
`Registry` is one of the requirements for closing that issue.
Please, share your thoughts on this plan.
--
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]
