gharris1727 commented on code in PR #16199: URL: https://github.com/apache/kafka/pull/16199#discussion_r1626609501
########## storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfig.java: ########## @@ -348,280 +346,124 @@ public final class RemoteLogManagerConfig { REMOTE_LOG_MANAGER_FETCH_QUOTA_WINDOW_SIZE_SECONDS_DOC); } - private final boolean enableRemoteStorageSystem; - private final String remoteStorageManagerClassName; - private final String remoteStorageManagerClassPath; - private final String remoteLogMetadataManagerClassName; - private final String remoteLogMetadataManagerClassPath; - private final long remoteLogIndexFileCacheTotalSizeBytes; - private final int remoteLogManagerThreadPoolSize; - private final int remoteLogManagerCopierThreadPoolSize; - private final int remoteLogManagerExpirationThreadPoolSize; - private final long remoteLogManagerTaskIntervalMs; - private final long remoteLogManagerTaskRetryBackoffMs; - private final long remoteLogManagerTaskRetryBackoffMaxMs; - private final double remoteLogManagerTaskRetryJitter; - private final int remoteLogReaderThreads; - private final int remoteLogReaderMaxPendingTasks; - private final String remoteStorageManagerPrefix; - private final HashMap<String, Object> remoteStorageManagerProps; - private final String remoteLogMetadataManagerPrefix; - private final HashMap<String, Object> remoteLogMetadataManagerProps; - private final String remoteLogMetadataManagerListenerName; - private final int remoteLogMetadataCustomMetadataMaxBytes; - private final long remoteLogManagerCopyMaxBytesPerSecond; - private final int remoteLogManagerCopyNumQuotaSamples; - private final int remoteLogManagerCopyQuotaWindowSizeSeconds; - private final long remoteLogManagerFetchMaxBytesPerSecond; - private final int remoteLogManagerFetchNumQuotaSamples; - private final int remoteLogManagerFetchQuotaWindowSizeSeconds; - - public RemoteLogManagerConfig(AbstractConfig config) { - this(config.getBoolean(REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP), - config.getString(REMOTE_STORAGE_MANAGER_CLASS_NAME_PROP), - config.getString(REMOTE_STORAGE_MANAGER_CLASS_PATH_PROP), - config.getString(REMOTE_LOG_METADATA_MANAGER_CLASS_NAME_PROP), - config.getString(REMOTE_LOG_METADATA_MANAGER_CLASS_PATH_PROP), - config.getString(REMOTE_LOG_METADATA_MANAGER_LISTENER_NAME_PROP), - config.getLong(REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP), - config.getInt(REMOTE_LOG_MANAGER_THREAD_POOL_SIZE_PROP), - config.getInt(REMOTE_LOG_MANAGER_COPIER_THREAD_POOL_SIZE_PROP), - config.getInt(REMOTE_LOG_MANAGER_EXPIRATION_THREAD_POOL_SIZE_PROP), - config.getLong(REMOTE_LOG_MANAGER_TASK_INTERVAL_MS_PROP), - config.getLong(REMOTE_LOG_MANAGER_TASK_RETRY_BACK_OFF_MS_PROP), - config.getLong(REMOTE_LOG_MANAGER_TASK_RETRY_BACK_OFF_MAX_MS_PROP), - config.getDouble(REMOTE_LOG_MANAGER_TASK_RETRY_JITTER_PROP), - config.getInt(REMOTE_LOG_READER_THREADS_PROP), - config.getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP), - config.getInt(REMOTE_LOG_METADATA_CUSTOM_METADATA_MAX_BYTES_PROP), - config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP), - config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) != null - ? config.originalsWithPrefix(config.getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)) - : Collections.emptyMap(), - config.getString(REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP), - config.getString(REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP) != null - ? config.originalsWithPrefix(config.getString(REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP)) - : Collections.emptyMap(), - config.getLong(REMOTE_LOG_MANAGER_COPY_MAX_BYTES_PER_SECOND_PROP), - config.getInt(REMOTE_LOG_MANAGER_COPY_QUOTA_WINDOW_NUM_PROP), - config.getInt(REMOTE_LOG_MANAGER_COPY_QUOTA_WINDOW_SIZE_SECONDS_PROP), - config.getLong(REMOTE_LOG_MANAGER_FETCH_MAX_BYTES_PER_SECOND_PROP), - config.getInt(REMOTE_LOG_MANAGER_FETCH_QUOTA_WINDOW_NUM_PROP), - config.getInt(REMOTE_LOG_MANAGER_FETCH_QUOTA_WINDOW_SIZE_SECONDS_PROP)); - } - - // Visible for testing - public RemoteLogManagerConfig(boolean enableRemoteStorageSystem, - String remoteStorageManagerClassName, - String remoteStorageManagerClassPath, - String remoteLogMetadataManagerClassName, - String remoteLogMetadataManagerClassPath, - String remoteLogMetadataManagerListenerName, - long remoteLogIndexFileCacheTotalSizeBytes, - int remoteLogManagerThreadPoolSize, - int remoteLogManagerCopierThreadPoolSize, - int remoteLogManagerExpirationThreadPoolSize, - long remoteLogManagerTaskIntervalMs, - long remoteLogManagerTaskRetryBackoffMs, - long remoteLogManagerTaskRetryBackoffMaxMs, - double remoteLogManagerTaskRetryJitter, - int remoteLogReaderThreads, - int remoteLogReaderMaxPendingTasks, - int remoteLogMetadataCustomMetadataMaxBytes, - String remoteStorageManagerPrefix, - Map<String, Object> remoteStorageManagerProps, /* properties having keys stripped out with remoteStorageManagerPrefix */ - String remoteLogMetadataManagerPrefix, - Map<String, Object> remoteLogMetadataManagerProps, /* properties having keys stripped out with remoteLogMetadataManagerPrefix */ - long remoteLogManagerCopyMaxBytesPerSecond, - int remoteLogManagerCopyNumQuotaSamples, - int remoteLogManagerCopyQuotaWindowSizeSeconds, - long remoteLogManagerFetchMaxBytesPerSecond, - int remoteLogManagerFetchNumQuotaSamples, - int remoteLogManagerFetchQuotaWindowSizeSeconds - ) { - this.enableRemoteStorageSystem = enableRemoteStorageSystem; - this.remoteStorageManagerClassName = remoteStorageManagerClassName; - this.remoteStorageManagerClassPath = remoteStorageManagerClassPath; - this.remoteLogMetadataManagerClassName = remoteLogMetadataManagerClassName; - this.remoteLogMetadataManagerClassPath = remoteLogMetadataManagerClassPath; - this.remoteLogIndexFileCacheTotalSizeBytes = remoteLogIndexFileCacheTotalSizeBytes; - this.remoteLogManagerThreadPoolSize = remoteLogManagerThreadPoolSize; - this.remoteLogManagerCopierThreadPoolSize = remoteLogManagerCopierThreadPoolSize; - this.remoteLogManagerExpirationThreadPoolSize = remoteLogManagerExpirationThreadPoolSize; - this.remoteLogManagerTaskIntervalMs = remoteLogManagerTaskIntervalMs; - this.remoteLogManagerTaskRetryBackoffMs = remoteLogManagerTaskRetryBackoffMs; - this.remoteLogManagerTaskRetryBackoffMaxMs = remoteLogManagerTaskRetryBackoffMaxMs; - this.remoteLogManagerTaskRetryJitter = remoteLogManagerTaskRetryJitter; - this.remoteLogReaderThreads = remoteLogReaderThreads; - this.remoteLogReaderMaxPendingTasks = remoteLogReaderMaxPendingTasks; - this.remoteStorageManagerPrefix = remoteStorageManagerPrefix; - this.remoteStorageManagerProps = new HashMap<>(remoteStorageManagerProps); - this.remoteLogMetadataManagerPrefix = remoteLogMetadataManagerPrefix; - this.remoteLogMetadataManagerProps = new HashMap<>(remoteLogMetadataManagerProps); - this.remoteLogMetadataManagerListenerName = remoteLogMetadataManagerListenerName; - this.remoteLogMetadataCustomMetadataMaxBytes = remoteLogMetadataCustomMetadataMaxBytes; - this.remoteLogManagerCopyMaxBytesPerSecond = remoteLogManagerCopyMaxBytesPerSecond; - this.remoteLogManagerCopyNumQuotaSamples = remoteLogManagerCopyNumQuotaSamples; - this.remoteLogManagerCopyQuotaWindowSizeSeconds = remoteLogManagerCopyQuotaWindowSizeSeconds; - this.remoteLogManagerFetchMaxBytesPerSecond = remoteLogManagerFetchMaxBytesPerSecond; - this.remoteLogManagerFetchNumQuotaSamples = remoteLogManagerFetchNumQuotaSamples; - this.remoteLogManagerFetchQuotaWindowSizeSeconds = remoteLogManagerFetchQuotaWindowSizeSeconds; - } - public boolean enableRemoteStorageSystem() { - return enableRemoteStorageSystem; + return getBoolean(REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP); } public String remoteStorageManagerClassName() { - return remoteStorageManagerClassName; + return getString(REMOTE_STORAGE_MANAGER_CLASS_NAME_PROP); } public String remoteStorageManagerClassPath() { - return remoteStorageManagerClassPath; + return getString(REMOTE_STORAGE_MANAGER_CLASS_PATH_PROP); } public String remoteLogMetadataManagerClassName() { - return remoteLogMetadataManagerClassName; + return getString(REMOTE_LOG_METADATA_MANAGER_CLASS_NAME_PROP); } public String remoteLogMetadataManagerClassPath() { - return remoteLogMetadataManagerClassPath; + return getString(REMOTE_LOG_METADATA_MANAGER_CLASS_PATH_PROP); } public long remoteLogIndexFileCacheTotalSizeBytes() { - return remoteLogIndexFileCacheTotalSizeBytes; + return getLong(REMOTE_LOG_INDEX_FILE_CACHE_TOTAL_SIZE_BYTES_PROP); } public int remoteLogManagerThreadPoolSize() { - return remoteLogManagerThreadPoolSize; + return getInt(REMOTE_LOG_MANAGER_THREAD_POOL_SIZE_PROP); } public int remoteLogManagerCopierThreadPoolSize() { - return remoteLogManagerCopierThreadPoolSize; + return getInt(REMOTE_LOG_MANAGER_COPIER_THREAD_POOL_SIZE_PROP); } public int remoteLogManagerExpirationThreadPoolSize() { - return remoteLogManagerExpirationThreadPoolSize; + return getInt(REMOTE_LOG_MANAGER_EXPIRATION_THREAD_POOL_SIZE_PROP); } public long remoteLogManagerTaskIntervalMs() { - return remoteLogManagerTaskIntervalMs; + return getLong(REMOTE_LOG_MANAGER_TASK_INTERVAL_MS_PROP); } public long remoteLogManagerTaskRetryBackoffMs() { - return remoteLogManagerTaskRetryBackoffMs; + return getLong(REMOTE_LOG_MANAGER_TASK_RETRY_BACK_OFF_MS_PROP); } public long remoteLogManagerTaskRetryBackoffMaxMs() { - return remoteLogManagerTaskRetryBackoffMaxMs; + return getLong(REMOTE_LOG_MANAGER_TASK_RETRY_BACK_OFF_MAX_MS_PROP); } public double remoteLogManagerTaskRetryJitter() { - return remoteLogManagerTaskRetryJitter; + return getDouble(REMOTE_LOG_MANAGER_TASK_RETRY_JITTER_PROP); } public int remoteLogReaderThreads() { - return remoteLogReaderThreads; + return getInt(REMOTE_LOG_READER_THREADS_PROP); } public int remoteLogReaderMaxPendingTasks() { - return remoteLogReaderMaxPendingTasks; + return getInt(REMOTE_LOG_READER_MAX_PENDING_TASKS_PROP); } public String remoteLogMetadataManagerListenerName() { - return remoteLogMetadataManagerListenerName; + return getString(REMOTE_LOG_METADATA_MANAGER_LISTENER_NAME_PROP); } public int remoteLogMetadataCustomMetadataMaxBytes() { - return remoteLogMetadataCustomMetadataMaxBytes; + return getInt(REMOTE_LOG_METADATA_CUSTOM_METADATA_MAX_BYTES_PROP); } public String remoteStorageManagerPrefix() { - return remoteStorageManagerPrefix; + return getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP); } public String remoteLogMetadataManagerPrefix() { - return remoteLogMetadataManagerPrefix; + return getString(REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP); } public Map<String, Object> remoteStorageManagerProps() { - return Collections.unmodifiableMap(remoteStorageManagerProps); + return Collections.unmodifiableMap(getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP) != null + ? originalsWithPrefix(getString(REMOTE_STORAGE_MANAGER_CONFIG_PREFIX_PROP)) + : Collections.emptyMap()); } public Map<String, Object> remoteLogMetadataManagerProps() { - return Collections.unmodifiableMap(remoteLogMetadataManagerProps); + return Collections.unmodifiableMap(getString(REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP) != null + ? originalsWithPrefix(getString(REMOTE_LOG_METADATA_MANAGER_CONFIG_PREFIX_PROP)) + : Collections.emptyMap()); } public long remoteLogManagerCopyMaxBytesPerSecond() { - return remoteLogManagerCopyMaxBytesPerSecond; + return getLong(REMOTE_LOG_MANAGER_COPY_MAX_BYTES_PER_SECOND_PROP); } public int remoteLogManagerCopyNumQuotaSamples() { - return remoteLogManagerCopyNumQuotaSamples; + return getInt(REMOTE_LOG_MANAGER_COPY_QUOTA_WINDOW_NUM_PROP); } public int remoteLogManagerCopyQuotaWindowSizeSeconds() { - return remoteLogManagerCopyQuotaWindowSizeSeconds; + return getInt(REMOTE_LOG_MANAGER_COPY_QUOTA_WINDOW_SIZE_SECONDS_PROP); } public long remoteLogManagerFetchMaxBytesPerSecond() { - return remoteLogManagerFetchMaxBytesPerSecond; + return getLong(REMOTE_LOG_MANAGER_FETCH_MAX_BYTES_PER_SECOND_PROP); } public int remoteLogManagerFetchNumQuotaSamples() { - return remoteLogManagerFetchNumQuotaSamples; + return getInt(REMOTE_LOG_MANAGER_FETCH_QUOTA_WINDOW_NUM_PROP); } public int remoteLogManagerFetchQuotaWindowSizeSeconds() { - return remoteLogManagerFetchQuotaWindowSizeSeconds; + return getInt(REMOTE_LOG_MANAGER_FETCH_QUOTA_WINDOW_SIZE_SECONDS_PROP); } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (!(o instanceof RemoteLogManagerConfig)) return false; - RemoteLogManagerConfig that = (RemoteLogManagerConfig) o; - return enableRemoteStorageSystem == that.enableRemoteStorageSystem - && remoteLogIndexFileCacheTotalSizeBytes == that.remoteLogIndexFileCacheTotalSizeBytes - && remoteLogManagerThreadPoolSize == that.remoteLogManagerThreadPoolSize - && remoteLogManagerCopierThreadPoolSize == that.remoteLogManagerCopierThreadPoolSize - && remoteLogManagerExpirationThreadPoolSize == that.remoteLogManagerExpirationThreadPoolSize - && remoteLogManagerTaskIntervalMs == that.remoteLogManagerTaskIntervalMs - && remoteLogManagerTaskRetryBackoffMs == that.remoteLogManagerTaskRetryBackoffMs - && remoteLogManagerTaskRetryBackoffMaxMs == that.remoteLogManagerTaskRetryBackoffMaxMs - && remoteLogManagerTaskRetryJitter == that.remoteLogManagerTaskRetryJitter - && remoteLogReaderThreads == that.remoteLogReaderThreads - && remoteLogReaderMaxPendingTasks == that.remoteLogReaderMaxPendingTasks - && remoteLogMetadataCustomMetadataMaxBytes == that.remoteLogMetadataCustomMetadataMaxBytes - && Objects.equals(remoteStorageManagerClassName, that.remoteStorageManagerClassName) - && Objects.equals(remoteStorageManagerClassPath, that.remoteStorageManagerClassPath) - && Objects.equals(remoteLogMetadataManagerClassName, that.remoteLogMetadataManagerClassName) - && Objects.equals(remoteLogMetadataManagerClassPath, that.remoteLogMetadataManagerClassPath) - && Objects.equals(remoteLogMetadataManagerListenerName, that.remoteLogMetadataManagerListenerName) - && Objects.equals(remoteStorageManagerProps, that.remoteStorageManagerProps) - && Objects.equals(remoteLogMetadataManagerProps, that.remoteLogMetadataManagerProps) - && Objects.equals(remoteStorageManagerPrefix, that.remoteStorageManagerPrefix) - && Objects.equals(remoteLogMetadataManagerPrefix, that.remoteLogMetadataManagerPrefix) - && remoteLogManagerCopyMaxBytesPerSecond == that.remoteLogManagerCopyMaxBytesPerSecond - && remoteLogManagerCopyNumQuotaSamples == that.remoteLogManagerCopyNumQuotaSamples - && remoteLogManagerCopyQuotaWindowSizeSeconds == that.remoteLogManagerCopyQuotaWindowSizeSeconds - && remoteLogManagerFetchMaxBytesPerSecond == that.remoteLogManagerFetchMaxBytesPerSecond - && remoteLogManagerFetchNumQuotaSamples == that.remoteLogManagerFetchNumQuotaSamples - && remoteLogManagerFetchQuotaWindowSizeSeconds == that.remoteLogManagerFetchQuotaWindowSizeSeconds; + public RemoteLogManagerConfig(Map<?, ?> props) { + this(props, false); } - @Override - public int hashCode() { - return Objects.hash(enableRemoteStorageSystem, remoteStorageManagerClassName, remoteStorageManagerClassPath, - remoteLogMetadataManagerClassName, remoteLogMetadataManagerClassPath, remoteLogMetadataManagerListenerName, - remoteLogMetadataCustomMetadataMaxBytes, remoteLogIndexFileCacheTotalSizeBytes, remoteLogManagerThreadPoolSize, - remoteLogManagerCopierThreadPoolSize, remoteLogManagerExpirationThreadPoolSize, remoteLogManagerTaskIntervalMs, - remoteLogManagerTaskRetryBackoffMs, remoteLogManagerTaskRetryBackoffMaxMs, remoteLogManagerTaskRetryJitter, - remoteLogReaderThreads, remoteLogReaderMaxPendingTasks, remoteStorageManagerProps, remoteLogMetadataManagerProps, - remoteStorageManagerPrefix, remoteLogMetadataManagerPrefix, remoteLogManagerCopyMaxBytesPerSecond, - remoteLogManagerCopyNumQuotaSamples, remoteLogManagerCopyQuotaWindowSizeSeconds, remoteLogManagerFetchMaxBytesPerSecond, - remoteLogManagerFetchNumQuotaSamples, remoteLogManagerFetchQuotaWindowSizeSeconds); + protected RemoteLogManagerConfig(Map<?, ?> props, boolean doLog) { Review Comment: This constructor isn't doing anything. You can call super(ConfigDef, Map) in the other constructor. Also don't forget to remove CONFIG_DEF and replace it with a ConfigDef configDef() static method. ########## storage/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerConfigTest.java: ########## @@ -27,171 +25,72 @@ import static org.apache.kafka.server.log.remote.storage.RemoteLogManagerConfig.DEFAULT_REMOTE_LOG_METADATA_MANAGER_CLASS_NAME; import static org.apache.kafka.server.log.remote.storage.RemoteLogManagerConfig.REMOTE_LOG_METADATA_MANAGER_CLASS_NAME_PROP; +import static org.apache.kafka.server.log.remote.storage.RemoteLogManagerConfig.REMOTE_LOG_METADATA_MANAGER_LISTENER_NAME_PROP; +import static org.apache.kafka.server.log.remote.storage.RemoteLogManagerConfig.REMOTE_LOG_STORAGE_SYSTEM_ENABLE_PROP; +import static org.apache.kafka.server.log.remote.storage.RemoteLogManagerConfig.REMOTE_STORAGE_MANAGER_CLASS_PATH_PROP; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotEquals; public class RemoteLogManagerConfigTest { - private static class TestConfig extends AbstractConfig { - public TestConfig(Map<?, ?> originals) { - super(RemoteLogManagerConfig.CONFIG_DEF, originals, true); - } - } - @ParameterizedTest @ValueSource(booleans = {true, false}) public void testValidConfigs(boolean useDefaultRemoteLogMetadataManagerClass) { String rsmPrefix = "__custom.rsm."; String rlmmPrefix = "__custom.rlmm."; Map<String, Object> rsmProps = Collections.singletonMap("rsm.prop", "val"); Map<String, Object> rlmmProps = Collections.singletonMap("rlmm.prop", "val"); - RemoteLogManagerConfig expectedRemoteLogManagerConfig = getRemoteLogManagerConfig(useDefaultRemoteLogMetadataManagerClass, - rsmPrefix, - rlmmPrefix, - rsmProps, - rlmmProps); - Map<String, Object> props = extractProps(expectedRemoteLogManagerConfig); + Map<String, Object> props = getRLMProps(useDefaultRemoteLogMetadataManagerClass); rsmProps.forEach((k, v) -> props.put(rsmPrefix + k, v)); rlmmProps.forEach((k, v) -> props.put(rlmmPrefix + k, v)); + + RemoteLogManagerConfig expectedRemoteLogManagerConfig = new RemoteLogManagerConfig(props); + // Removing remote.log.metadata.manager.class.name so that the default value gets picked up. if (useDefaultRemoteLogMetadataManagerClass) { props.remove(REMOTE_LOG_METADATA_MANAGER_CLASS_NAME_PROP); } Review Comment: This is such an incredibly silly test now that we're using AbstractConfig. Of course the default will be applied if the custom value isn't present in the map, that's what AbstractConfig is useful for! If `useDefaultRemoteLogMetadataManagerClass` is false, then this test reduces to `assertEquals(new RemoteLogManagerConfig(props).values(), new RemoteLogManagerConfig(props).values());` which is also very low-value. I think removing the parameterization, and confirming that the RLMC can be instantiated once is more than sufficient here. You should probably add an assertion for the rsmProps and rlmmProps, comparing them to the output of the non-trivial methods in the RLMC, since that is coverage that we are losing in this refactor. If you want, you can also add a test that constructs an RLMC with an empty configuration, because there are default values specified for all configurations. If you are really generous with your time, you can also add some bad values and expect ConfigExceptions. A few of the configurations have NonEmptyString validators, so you could verify those are present with tests. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org