You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "tkalkirill (via GitHub)" <gi...@apache.org> on 2024/10/24 06:27:13 UTC
[PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
tkalkirill opened a new pull request, #4623:
URL: https://github.com/apache/ignite-3/pull/4623
https://issues.apache.org/jira/browse/IGNITE-23279
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1820953616
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/CompactionRevisionUpdateListener.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.ignite.internal.metastorage;
+
+/**
+ * Listener which receives and handles the metastorage compaction revision update after start watches.
Review Comment:
after watches have been started
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816533644
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
Review Comment:
Could you please mention this specifically in the Javadoc please?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816536106
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
Review Comment:
>Also, this value will be subtracted from the current safe time, if during the repair the safe time does not move, then everything will be ok.
As soon as new leader comes to life, it will update its SafeTime to the current clock time and compact out everything that is older than 1 hour. This doesn't seem to change anything in the provided scenario.
Could we choose 24 hours as a default? Seems to be sufficient.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816088421
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
Review Comment:
When doing MG disaster recovery there is a scenario:
1. Old leader A (the only MG voting member) crashes and cannot start again
2. User repairs MG on remaining nodes
3. Later, old ex-leader from item 1 is migrated to the repaired cluster.
To successfully join, A needs to check its Metastorage checksum against the new leader's checksums. If the default for MS data availability is just 1 hour and more than 1 hour passed between items 2 and 3, the new leader could have already compacted out the latest MS revision which A has, so A will not be able to join.
Could this default be increased?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
Review Comment:
Is this an interval between compaction initiations (or maybe checks to see whether a compaction is needed)?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
+ public static final long DATA_AVAILABILITY_TIME_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ private final ReadWriteLock rwLock = new ReentrantReadWriteLock();
+
+ /** Guarded by {@link #rwLock}. */
+ private long interval;
+
+ /** Guarded by {@link #rwLock}. */
+ private long dataAvailabilityTime;
+
+ /** Constructor. */
+ public MetaStorageCompactionTriggerConfiguration(SystemDistributedConfiguration systemDistributedConfiguration) {
+ updateSystemProperties(systemDistributedConfiguration.value());
+
+ systemDistributedConfiguration.listen(ctx -> {
Review Comment:
Is a race between the initial update and subscribing to events possible?
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfigurationTest.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/** For {@link MetaStorageCompactionTriggerConfiguration} testing. */
+@ExtendWith(ConfigurationExtension.class)
+public class MetaStorageCompactionTriggerConfigurationTest extends BaseIgniteAbstractTest {
+ private static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ private static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ private static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ private static final long DATA_AVAILABILITY_TIME_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ @Test
+ void testEmptySystemProperties(@InjectConfiguration SystemDistributedConfiguration systemConfig) {
+ var config = new MetaStorageCompactionTriggerConfiguration(systemConfig);
+
+ assertEquals(INTERVAL_DEFAULT_VALUE, config.interval());
+ assertEquals(DATA_AVAILABILITY_TIME_DEFAULT_VALUE, config.dataAvailabilityTime());
+ }
+
+ @Test
+ void testInvalidSystemPropertiesOnStart(
+ @InjectConfiguration("mock.properties = {"
+ + INTERVAL_SYSTEM_PROPERTY_NAME + ".propertyValue = foo, "
+ + DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME + ".propertyValue = bar"
Review Comment:
```suggestion
+ INTERVAL_SYSTEM_PROPERTY_NAME + ".propertyValue = not-a-long, "
+ DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME + ".propertyValue = not-a-long"
```
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
Review Comment:
The term 'system property' is already taken by Java system properties. At least, when I started reading this class, I was confused. Can another term be devised? Like 'internal property'?
##########
modules/metastorage/src/test/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfigurationTest.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.concurrent.CompletableFuture;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/** For {@link MetaStorageCompactionTriggerConfiguration} testing. */
+@ExtendWith(ConfigurationExtension.class)
+public class MetaStorageCompactionTriggerConfigurationTest extends BaseIgniteAbstractTest {
+ private static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ private static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ private static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ private static final long DATA_AVAILABILITY_TIME_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ @Test
+ void testEmptySystemProperties(@InjectConfiguration SystemDistributedConfiguration systemConfig) {
+ var config = new MetaStorageCompactionTriggerConfiguration(systemConfig);
+
+ assertEquals(INTERVAL_DEFAULT_VALUE, config.interval());
+ assertEquals(DATA_AVAILABILITY_TIME_DEFAULT_VALUE, config.dataAvailabilityTime());
+ }
+
+ @Test
+ void testInvalidSystemPropertiesOnStart(
+ @InjectConfiguration("mock.properties = {"
+ + INTERVAL_SYSTEM_PROPERTY_NAME + ".propertyValue = foo, "
+ + DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME + ".propertyValue = bar"
+ + "}")
+ SystemDistributedConfiguration systemConfig
+ ) {
+ var config = new MetaStorageCompactionTriggerConfiguration(systemConfig);
+
+ assertEquals(INTERVAL_DEFAULT_VALUE, config.interval());
+ assertEquals(DATA_AVAILABILITY_TIME_DEFAULT_VALUE, config.dataAvailabilityTime());
+ }
+
+ @Test
+ void testValidSystemPropertiesOnStart(
+ @InjectConfiguration("mock.properties = {"
+ + INTERVAL_SYSTEM_PROPERTY_NAME + ".propertyValue = \"100\", "
+ + DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME + ".propertyValue = \"500\""
+ + "}")
+ SystemDistributedConfiguration systemConfig
+ ) {
+ var config = new MetaStorageCompactionTriggerConfiguration(systemConfig);
+
+ assertEquals(100, config.interval());
+ assertEquals(500, config.dataAvailabilityTime());
+ }
+
+ @Test
+ void testInvalidSystemPropertiesOnChange(@InjectConfiguration SystemDistributedConfiguration systemConfig) {
+ var config = new MetaStorageCompactionTriggerConfiguration(systemConfig);
+
+ changeSystemConfig(systemConfig, "foo", "bar");
Review Comment:
```suggestion
changeSystemConfig(systemConfig, "not-a-long", "not-a-long");
```
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
Review Comment:
Or maybe use other 'checksum availabilityy time' which is significantly larger than the 'data availability time'?
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
+ public static final long DATA_AVAILABILITY_TIME_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ private final ReadWriteLock rwLock = new ReentrantReadWriteLock();
+
+ /** Guarded by {@link #rwLock}. */
+ private long interval;
+
+ /** Guarded by {@link #rwLock}. */
+ private long dataAvailabilityTime;
+
+ /** Constructor. */
+ public MetaStorageCompactionTriggerConfiguration(SystemDistributedConfiguration systemDistributedConfiguration) {
+ updateSystemProperties(systemDistributedConfiguration.value());
+
+ systemDistributedConfiguration.listen(ctx -> {
+ updateSystemProperties(ctx.newValue());
+
+ return nullCompletedFuture();
+ });
+ }
+
+ /** Returns compaction start interval (in milliseconds). */
+ long interval() {
+ rwLock.readLock().lock();
+
+ try {
+ return interval;
+ } finally {
+ rwLock.readLock().unlock();
+ }
+ }
+
+ /** Data availability time (in milliseconds). */
+ long dataAvailabilityTime() {
+ rwLock.readLock().lock();
+
+ try {
+ return dataAvailabilityTime;
+ } finally {
+ rwLock.readLock().unlock();
+ }
+ }
+
+ private void updateSystemProperties(SystemDistributedView view) {
+ rwLock.writeLock().lock();
+
+ try {
+ interval = longValue(view, INTERVAL_SYSTEM_PROPERTY_NAME, INTERVAL_DEFAULT_VALUE);
+ dataAvailabilityTime = longValue(view, DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME, DATA_AVAILABILITY_TIME_DEFAULT_VALUE);
+ } finally {
+ rwLock.writeLock().unlock();
+ }
+ }
+
+ private static long longValue(SystemDistributedView systemDistributedView, String systemPropertyName, long defaultValue) {
+ return longValue(systemDistributedView.properties().get(systemPropertyName), systemPropertyName, defaultValue);
+ }
+
+ private static long longValue(@Nullable SystemPropertyView systemPropertyView, String systemPropertyName, long defaultValue) {
+ if (systemPropertyView == null) {
+ return defaultValue;
+ }
+
+ try {
+ return Long.parseLong(systemPropertyView.propertyValue());
+ } catch (NumberFormatException e) {
+ LOG.warn(
Review Comment:
This looks dangerous. On the other hand, we cannot fail startup if there is something unparseable as this is a distributed config and we'll break the cluster. Do we have a common approach to such situations?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816537526
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
+ public static final long DATA_AVAILABILITY_TIME_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ private final ReadWriteLock rwLock = new ReentrantReadWriteLock();
+
+ /** Guarded by {@link #rwLock}. */
+ private long interval;
+
+ /** Guarded by {@link #rwLock}. */
+ private long dataAvailabilityTime;
+
+ /** Constructor. */
+ public MetaStorageCompactionTriggerConfiguration(SystemDistributedConfiguration systemDistributedConfiguration) {
+ updateSystemProperties(systemDistributedConfiguration.value());
+
+ systemDistributedConfiguration.listen(ctx -> {
+ updateSystemProperties(ctx.newValue());
+
+ return nullCompletedFuture();
+ });
+ }
+
+ /** Returns compaction start interval (in milliseconds). */
+ long interval() {
+ rwLock.readLock().lock();
+
+ try {
+ return interval;
+ } finally {
+ rwLock.readLock().unlock();
+ }
+ }
+
+ /** Data availability time (in milliseconds). */
+ long dataAvailabilityTime() {
+ rwLock.readLock().lock();
+
+ try {
+ return dataAvailabilityTime;
+ } finally {
+ rwLock.readLock().unlock();
+ }
+ }
+
+ private void updateSystemProperties(SystemDistributedView view) {
+ rwLock.writeLock().lock();
+
+ try {
+ interval = longValue(view, INTERVAL_SYSTEM_PROPERTY_NAME, INTERVAL_DEFAULT_VALUE);
+ dataAvailabilityTime = longValue(view, DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME, DATA_AVAILABILITY_TIME_DEFAULT_VALUE);
+ } finally {
+ rwLock.writeLock().unlock();
+ }
+ }
+
+ private static long longValue(SystemDistributedView systemDistributedView, String systemPropertyName, long defaultValue) {
+ return longValue(systemDistributedView.properties().get(systemPropertyName), systemPropertyName, defaultValue);
+ }
+
+ private static long longValue(@Nullable SystemPropertyView systemPropertyView, String systemPropertyName, long defaultValue) {
+ if (systemPropertyView == null) {
+ return defaultValue;
+ }
+
+ try {
+ return Long.parseLong(systemPropertyView.propertyValue());
+ } catch (NumberFormatException e) {
+ LOG.warn(
Review Comment:
If I'm not mistaken, as a result of a discussion it was decided to add validation
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816204303
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
Review Comment:
Yes, between the compaction initiations.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816536499
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
+ public static final long DATA_AVAILABILITY_TIME_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ private final ReadWriteLock rwLock = new ReentrantReadWriteLock();
+
+ /** Guarded by {@link #rwLock}. */
+ private long interval;
+
+ /** Guarded by {@link #rwLock}. */
+ private long dataAvailabilityTime;
+
+ /** Constructor. */
+ public MetaStorageCompactionTriggerConfiguration(SystemDistributedConfiguration systemDistributedConfiguration) {
+ updateSystemProperties(systemDistributedConfiguration.value());
+
+ systemDistributedConfiguration.listen(ctx -> {
Review Comment:
Please add a comment explaining this
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1818948989
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
Review Comment:
Try fix 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816184650
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
+ public static final long DATA_AVAILABILITY_TIME_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ private final ReadWriteLock rwLock = new ReentrantReadWriteLock();
+
+ /** Guarded by {@link #rwLock}. */
+ private long interval;
+
+ /** Guarded by {@link #rwLock}. */
+ private long dataAvailabilityTime;
+
+ /** Constructor. */
+ public MetaStorageCompactionTriggerConfiguration(SystemDistributedConfiguration systemDistributedConfiguration) {
+ updateSystemProperties(systemDistributedConfiguration.value());
+
+ systemDistributedConfiguration.listen(ctx -> {
Review Comment:
I don't think so, we will use it in the constructor when creating a trigger, and we will receive a notification after deploying watches.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1820885387
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
Review Comment:
A network partition that is a couple of hours long is a lot more probable than a network partition of 1 day long. By setting this to 1 day, we solve a lot of possible repair problems, and we cannot solve them all as we have compaction.
> The meaning of this property was put on the PDS and no one left a comment.
At that time no one thought about this, now we have a scenario.
Let's not make this a blocker for merging this, but let's file a separate issue if you don't want to change it in this PR.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816219369
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
Review Comment:
This value is taken randomly, I just thought that this is more or less a sufficient value.
We can increase it, or wait for real bad cases and empirically understand what value is needed.
Also, this value will be subtracted from the current safe time, if during the repair the safe time does not move, then everything will be ok.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816198529
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
+ public static final long DATA_AVAILABILITY_TIME_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ private final ReadWriteLock rwLock = new ReentrantReadWriteLock();
+
+ /** Guarded by {@link #rwLock}. */
+ private long interval;
+
+ /** Guarded by {@link #rwLock}. */
+ private long dataAvailabilityTime;
+
+ /** Constructor. */
+ public MetaStorageCompactionTriggerConfiguration(SystemDistributedConfiguration systemDistributedConfiguration) {
+ updateSystemProperties(systemDistributedConfiguration.value());
+
+ systemDistributedConfiguration.listen(ctx -> {
+ updateSystemProperties(ctx.newValue());
+
+ return nullCompletedFuture();
+ });
+ }
+
+ /** Returns compaction start interval (in milliseconds). */
+ long interval() {
+ rwLock.readLock().lock();
+
+ try {
+ return interval;
+ } finally {
+ rwLock.readLock().unlock();
+ }
+ }
+
+ /** Data availability time (in milliseconds). */
+ long dataAvailabilityTime() {
+ rwLock.readLock().lock();
+
+ try {
+ return dataAvailabilityTime;
+ } finally {
+ rwLock.readLock().unlock();
+ }
+ }
+
+ private void updateSystemProperties(SystemDistributedView view) {
+ rwLock.writeLock().lock();
+
+ try {
+ interval = longValue(view, INTERVAL_SYSTEM_PROPERTY_NAME, INTERVAL_DEFAULT_VALUE);
+ dataAvailabilityTime = longValue(view, DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME, DATA_AVAILABILITY_TIME_DEFAULT_VALUE);
+ } finally {
+ rwLock.writeLock().unlock();
+ }
+ }
+
+ private static long longValue(SystemDistributedView systemDistributedView, String systemPropertyName, long defaultValue) {
+ return longValue(systemDistributedView.properties().get(systemPropertyName), systemPropertyName, defaultValue);
+ }
+
+ private static long longValue(@Nullable SystemPropertyView systemPropertyView, String systemPropertyName, long defaultValue) {
+ if (systemPropertyView == null) {
+ return defaultValue;
+ }
+
+ try {
+ return Long.parseLong(systemPropertyView.propertyValue());
+ } catch (NumberFormatException e) {
+ LOG.warn(
Review Comment:
It doesn't look dangerous to me. I thought about it and I think it's better not to drop the unit if the internal configuration was changed by mistake.
> Do we have a common approach to such situations?
No, this is the first such case.
I can try to discuss it with a larger group if you think it's necessary.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1818956107
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
Review Comment:
Fix 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1820890653
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/CompactionRevisionUpdateListener.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.ignite.internal.metastorage;
+
+/**
+ * Listener which receives and handles the metastorage compaction revision update after start watches.
+ *
+ * <p>It is guaranteed that it will <b>not</b> invoked in parallel with the handling of watch events or metastore revision update
Review Comment:
```suggestion
* <p>It is guaranteed that it will <b>not</b> be invoked in parallel with the handling of watch events or metastore revision update
```
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/CompactionRevisionUpdateListener.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.ignite.internal.metastorage;
+
+/**
+ * Listener which receives and handles the metastorage compaction revision update after start watches.
Review Comment:
'After starting watches' / 'after watches have been started'?
##########
modules/metastorage-api/src/main/java/org/apache/ignite/internal/metastorage/CompactionRevisionUpdateListener.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.ignite.internal.metastorage;
+
+/**
+ * Listener which receives and handles the metastorage compaction revision update after start watches.
+ *
+ * <p>It is guaranteed that it will <b>not</b> invoked in parallel with the handling of watch events or metastore revision update
+ * events.</p>
+ */
+@FunctionalInterface
+public interface CompactionRevisionUpdateListener {
+ /**
+ * Invokes when the metastorage compaction revision has been updated in the WatchEvent queue.
Review Comment:
```suggestion
* Invoked when the metastorage compaction revision has been updated in the WatchEvent queue.
```
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTrigger.java:
##########
@@ -92,15 +88,15 @@ class MetaStorageCompactionTrigger implements ManuallyCloseable {
/** Guarded by {@link #lock}. */
private boolean isLocalNodeLeader;
- private final Lock lock = new ReentrantLock();
-
- /** Compaction start interval (in milliseconds). */
- // TODO: IGNITE-23279 Change configuration
- private final long startInterval = IgniteSystemProperties.getLong(COMPACTION_INTERVAL_PROPERTY, Long.MAX_VALUE);
+ /**
+ * Flag that the component has started. It is expected that the component will be started after the distributed configuration has
Review Comment:
```suggestion
* Whether the component has started. It is expected that the component will be started after the distributed configuration has
```
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTrigger.java:
##########
@@ -258,7 +282,7 @@ private void onCompactionRevisionUpdateBusy(long compactionRevision) {
}
/** Invokes when a new leader is elected. */
Review Comment:
```suggestion
/** Invoked when a new leader is elected. */
```
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTrigger.java:
##########
@@ -230,7 +254,7 @@ private void scheduleNextCompactionBusy() {
}
/** Invokes when the metastorage compaction revision is updated. */
Review Comment:
```suggestion
/** Invoked when the metastorage compaction revision is updated. */
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1816201602
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
Review Comment:
I prefer the term system property.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1818954934
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
Review Comment:
In my opinion, a lot of garbage can accumulate in the metastorage during this time.
Why do you think that the repair can take more than an hour? Why not more than a day?
The meaning of this property was put on the PDS and no one left a comment.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1818948989
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
Review Comment:
Try fix 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23279 Add Metastorage Compaction Trigger Configuration [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4623:
URL: https://github.com/apache/ignite-3/pull/4623#discussion_r1820904642
##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/impl/MetaStorageCompactionTriggerConfiguration.java:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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.ignite.internal.metastorage.impl;
+
+import static org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.ignite.internal.configuration.SystemDistributedConfiguration;
+import org.apache.ignite.internal.configuration.SystemDistributedView;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.jetbrains.annotations.Nullable;
+
+/** Configuration for metastorage compaction triggering based on distributed system properties. */
+public class MetaStorageCompactionTriggerConfiguration {
+ private static final IgniteLogger LOG = Loggers.forClass(MetaStorageCompactionTriggerConfiguration.class);
+
+ /**
+ * System property that defines compaction start interval (in milliseconds).
+ *
+ * <p>Default value is {@link #INTERVAL_DEFAULT_VALUE}.</p>
+ */
+ public static final String INTERVAL_SYSTEM_PROPERTY_NAME = "metastorageCompactionInterval";
+
+ /**
+ * System property that defines compaction data availability time (in milliseconds).
+ *
+ * <p>Default value is {@link #DATA_AVAILABILITY_TIME_DEFAULT_VALUE}.</p>
+ */
+ public static final String DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME = "metastorageCompactionDataAvailabilityTime";
+
+ /** Default value for the {@link #INTERVAL_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 minute
+ public static final long INTERVAL_DEFAULT_VALUE = Long.MAX_VALUE;
+
+ /** Default value for the {@link #DATA_AVAILABILITY_TIME_SYSTEM_PROPERTY_NAME}. */
+ // TODO: IGNITE-23280 Make default 1 hour
Review Comment:
Created https://issues.apache.org/jira/browse/IGNITE-23573
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org