You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "jerqi (via GitHub)" <gi...@apache.org> on 2023/02/09 07:56:38 UTC
[GitHub] [incubator-uniffle] jerqi opened a new pull request, #572: [#410] fea: Support the hot reload of coordinator's configuration
jerqi opened a new pull request, #572:
URL: https://github.com/apache/incubator-uniffle/pull/572
### What changes were proposed in this pull request?
I refer to Hadoop implement. I design a hot reload process. We support nodeMax at first.
### Why are the changes needed?
It's more convenient.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
UT
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#issuecomment-1423822104
> Is this ready for review?
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#issuecomment-1423842667
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#572](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60db47a) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/f461460c1e8090082c7f83851fa99f72fec4f4ed?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f461460) will **decrease** coverage by `0.62%`.
> The diff coverage is `20.28%`.
```diff
@@ Coverage Diff @@
## master #572 +/- ##
============================================
- Coverage 60.87% 60.25% -0.62%
+ Complexity 1796 1569 -227
============================================
Files 213 192 -21
Lines 12375 10595 -1780
Branches 1049 893 -156
============================================
- Hits 7533 6384 -1149
+ Misses 4435 3847 -588
+ Partials 407 364 -43
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ache/uniffle/common/config/ReconfigurableBase.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9jb25maWcvUmVjb25maWd1cmFibGVCYXNlLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [.../org/apache/uniffle/coordinator/AccessManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQWNjZXNzTWFuYWdlci5qYXZh) | `71.73% <0.00%> (-22.55%)` | :arrow_down: |
| [.../apache/uniffle/coordinator/CoordinatorServer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ29vcmRpbmF0b3JTZXJ2ZXIuamF2YQ==) | `57.02% <23.07%> (-4.45%)` | :arrow_down: |
| [...ache/uniffle/coordinator/SimpleClusterManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvU2ltcGxlQ2x1c3Rlck1hbmFnZXIuamF2YQ==) | `82.83% <25.00%> (-3.89%)` | :arrow_down: |
| [...nator/access/checker/AccessClusterLoadChecker.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvYWNjZXNzL2NoZWNrZXIvQWNjZXNzQ2x1c3RlckxvYWRDaGVja2VyLmphdmE=) | `82.50% <33.33%> (-14.47%)` | :arrow_down: |
| [.../org/apache/uniffle/common/config/RssBaseConf.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9jb25maWcvUnNzQmFzZUNvbmYuamF2YQ==) | `94.04% <100.00%> (+0.22%)` | :arrow_up: |
| [...ava/org/apache/uniffle/server/ShuffleTaskInfo.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlVGFza0luZm8uamF2YQ==) | `98.07% <0.00%> (-1.93%)` | :arrow_down: |
| [...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL01SSWRIZWxwZXIuamF2YQ==) | | |
| [...pache/hadoop/mapreduce/task/reduce/RssFetcher.java](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0ZldGNoZXIuamF2YQ==) | | |
| ... and [20 more](https://codecov.io/gh/apache/incubator-uniffle/pull/572?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#issuecomment-1425256926
Thanks @advancedxy @kaijchen , merged.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101189135
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ long newLastModify = new File(rssConf.getString(RECONFIGURABLE_FILE_NAME, "")).lastModified();
Review Comment:
OK, I will correct it. It must be set actually.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#issuecomment-1423819717
Is this ready for review?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101214602
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java:
##########
@@ -58,7 +60,7 @@ public class SimpleClusterManager implements ClusterManager {
private Map<String, Set<ServerNode>> tagToNodes = Maps.newConcurrentMap();
private AtomicLong excludeLastModify = new AtomicLong(0L);
private long heartbeatTimeout;
- private int shuffleNodesMax;
+ private AtomicInteger shuffleNodesMax;
Review Comment:
But I find this https://www.zhihu.com/question/329746124, I still have concern about it. I don't think atomicInteger will bring too much cost.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1102247942
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private final ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private final AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ String fileName = rssConf.getString(RECONFIGURABLE_FILE_NAME, "");
+ if (fileName.isEmpty()) {
+ LOG.warn("Config file name isn't set, we skip checking");
+ return;
+ }
+ File configFile = new File(fileName);
+ if (!configFile.exists()) {
+ LOG.warn("Config file doesn't exist, we skip checking");
+ return;
+ }
+ long newLastModify = configFile.lastModified();
+ if (lastModify.get() == 0) {
+ lastModify.set(newLastModify);
+ return;
+ }
+ if (newLastModify != lastModify.get()) {
+ LOG.warn("Server detect the modification of file, we start to reconfigure");
Review Comment:
Seems this is not resolved yet. Are you going to address it or just keep 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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1102272891
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private final ScheduledExecutorService scheduledExecutorService;
+ private final long checkIntervalSec;
+ private final AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ this::checkConfiguration, checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ String fileName = rssConf.getString(RECONFIGURABLE_FILE_NAME, "");
+ if (fileName.isEmpty()) {
+ LOG.warn("Config file name isn't set, we skip checking");
+ return;
+ }
+ File configFile = new File(fileName);
+ if (!configFile.exists()) {
+ LOG.warn("Config file doesn't exist, we skip checking");
+ return;
+ }
+ long newLastModify = configFile.lastModified();
+ if (lastModify.get() == 0) {
+ lastModify.set(newLastModify);
+ return;
+ }
+ if (newLastModify != lastModify.get()) {
+ LOG.warn("Server detect the modification of file " + fileName + ", we start to reconfigure");
Review Comment:
```suggestion
LOG.warn("Server detect the modification of file {}, we start to reconfigure", fileName);
```
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101196936
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ long newLastModify = new File(rssConf.getString(RECONFIGURABLE_FILE_NAME, "")).lastModified();
+ if (lastModify.get() == 0) {
+ lastModify.set(newLastModify);
+ return;
+ }
+ if (newLastModify != lastModify.get()) {
Review Comment:
fair point.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1102201013
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/access/checker/AccessClusterLoadChecker.java:
##########
@@ -39,15 +41,15 @@
* AccessClusterLoadChecker use the cluster load metrics including memory and healthy to
* filter and count available nodes numbers and reject if the number do not reach the threshold.
*/
-public class AccessClusterLoadChecker extends AbstractAccessChecker {
+public class AccessClusterLoadChecker extends AbstractAccessChecker implements Reconfigurable {
private static final Logger LOG = LoggerFactory.getLogger(AccessClusterLoadChecker.class);
private final ClusterManager clusterManager;
private final double memoryPercentThreshold;
// The hard constraint number of available shuffle servers
private final int availableServerNumThreshold;
- private final int defaultRequiredShuffleServerNumber;
+ private volatile int defaultRequiredShuffleServerNumber;
Review Comment:
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi merged pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101183992
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java:
##########
@@ -58,7 +60,7 @@ public class SimpleClusterManager implements ClusterManager {
private Map<String, Set<ServerNode>> tagToNodes = Maps.newConcurrentMap();
private AtomicLong excludeLastModify = new AtomicLong(0L);
private long heartbeatTimeout;
- private int shuffleNodesMax;
+ private AtomicInteger shuffleNodesMax;
Review Comment:
No, we can't guarantee the atomicity of read data. We can read 2 new bytes and 2 old bytes.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101416813
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private final ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private final AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ String fileName = rssConf.getString(RECONFIGURABLE_FILE_NAME, "");
+ if (fileName.isEmpty()) {
+ LOG.warn("Config file name isn't set, we skip checking");
+ return;
+ }
+ File configFile = new File(fileName);
+ if (!configFile.exists()) {
+ LOG.warn("Config file doesn't exist, we skip checking");
+ return;
+ }
+ long newLastModify = configFile.lastModified();
+ if (lastModify.get() == 0) {
+ lastModify.set(newLastModify);
+ return;
+ }
+ if (newLastModify != lastModify.get()) {
+ LOG.warn("Server detect the modification of file, we start to reconfigure");
Review Comment:
nit: let's also logs the filename(including path) here.
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/access/checker/AccessClusterLoadChecker.java:
##########
@@ -39,15 +41,15 @@
* AccessClusterLoadChecker use the cluster load metrics including memory and healthy to
* filter and count available nodes numbers and reject if the number do not reach the threshold.
*/
-public class AccessClusterLoadChecker extends AbstractAccessChecker {
+public class AccessClusterLoadChecker extends AbstractAccessChecker implements Reconfigurable {
private static final Logger LOG = LoggerFactory.getLogger(AccessClusterLoadChecker.class);
private final ClusterManager clusterManager;
private final double memoryPercentThreshold;
// The hard constraint number of available shuffle servers
private final int availableServerNumThreshold;
- private final int defaultRequiredShuffleServerNumber;
+ private volatile int defaultRequiredShuffleServerNumber;
Review Comment:
nit: extra spece after `int `
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101188260
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ long newLastModify = new File(rssConf.getString(RECONFIGURABLE_FILE_NAME, "")).lastModified();
+ if (lastModify.get() == 0) {
+ lastModify.set(newLastModify);
+ return;
+ }
+ if (newLastModify != lastModify.get()) {
Review Comment:
It's more safe to use `!=`. If anyone modify the system clock, it will be effective, too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101198317
##########
common/src/main/java/org/apache/uniffle/common/config/Reconfigurable.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * 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.uniffle.common.config;
+
+public interface Reconfigurable {
+
+ void reconfigure(RssConf conf);
+
+ boolean isPropertyReconfigurable(String property);
Review Comment:
I refer to hadoop's implement. Although it's not used now, I feel it's useful.It's ok for add a properties for ConfigOptionBuilder. I can add a `todo` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101203646
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java:
##########
@@ -58,7 +60,7 @@ public class SimpleClusterManager implements ClusterManager {
private Map<String, Set<ServerNode>> tagToNodes = Maps.newConcurrentMap();
private AtomicLong excludeLastModify = new AtomicLong(0L);
private long heartbeatTimeout;
- private int shuffleNodesMax;
+ private AtomicInteger shuffleNodesMax;
Review Comment:
> No, we can't guarantee the atomicity of read data. We can read 2 new bytes and 2 old bytes.
https://docs.oracle.com/javase/specs/jls/se8/html/jls-8.html#jls-8.3.1.4 see this. You may confused Java and C/C++'s volatile memory modeling?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101673914
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private final ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private final AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
Review Comment:
```suggestion
this::checkConfiguration, checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
```
##########
integration-test/common/src/test/java/org/apache/uniffle/test/CoordinatorAssignmentTest.java:
##########
@@ -142,4 +152,28 @@ public void testAssignmentServerNodesNumber() throws Exception {
info = shuffleWriteClient.getShuffleAssignments("app1", 0, 10, 1, TAGS, SERVER_NUM - 1, -1);
assertEquals(SHUFFLE_NODES_MAX - 1, info.getServerToPartitionRanges().keySet().size());
}
+
+ @Test
+ public void testReconfigureNodeMax() throws Exception {
+ String fileName = coordinators.get(0).getCoordinatorConf()
+ .getString(ReconfigurableBase.RECONFIGURABLE_FILE_NAME,"");
+ new File(fileName).createNewFile();
Review Comment:
```suggestion
assertTrue(new File(fileName).createNewFile());
```
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private final ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
Review Comment:
```suggestion
private final long checkIntervalSec;
```
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1102200571
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private final ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private final AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ String fileName = rssConf.getString(RECONFIGURABLE_FILE_NAME, "");
+ if (fileName.isEmpty()) {
+ LOG.warn("Config file name isn't set, we skip checking");
+ return;
+ }
+ File configFile = new File(fileName);
+ if (!configFile.exists()) {
+ LOG.warn("Config file doesn't exist, we skip checking");
+ return;
+ }
+ long newLastModify = configFile.lastModified();
+ if (lastModify.get() == 0) {
+ lastModify.set(newLastModify);
+ return;
+ }
+ if (newLastModify != lastModify.get()) {
+ LOG.warn("Server detect the modification of file, we start to reconfigure");
Review Comment:
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101154710
##########
common/src/main/java/org/apache/uniffle/common/config/Reconfigurable.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * 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.uniffle.common.config;
+
+public interface Reconfigurable {
+
+ void reconfigure(RssConf conf);
+
+ boolean isPropertyReconfigurable(String property);
Review Comment:
is it possible to mark the properties is reloadable or not in the ConfigOptionBuilder, which seems more nature.
Also, for this PR, I didn't see this method is used anywhere?
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ long newLastModify = new File(rssConf.getString(RECONFIGURABLE_FILE_NAME, "")).lastModified();
Review Comment:
If reconfigurable_file_name is not set, we should skip this check.
Otherwise, it may cause some problems?
##########
common/src/main/java/org/apache/uniffle/common/config/ReconfigurableBase.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.uniffle.common.config;
+
+import java.io.File;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.uniffle.common.util.ThreadUtils;
+
+public abstract class ReconfigurableBase implements Reconfigurable {
+
+ private static final Logger LOG = LoggerFactory.getLogger(ReconfigurableBase.class);
+ public static final String RECONFIGURABLE_FILE_NAME = "reconfigurable.file.name";
+
+ private final RssConf rssConf;
+
+ private ScheduledExecutorService scheduledExecutorService;
+ private long checkIntervalSec;
+ private AtomicLong lastModify = new AtomicLong(0L);
+
+ public ReconfigurableBase(RssConf rssConf) {
+ this.rssConf = rssConf;
+ scheduledExecutorService = Executors.newSingleThreadScheduledExecutor(
+ ThreadUtils.getThreadFactory("ReconfigurableThread-%d"));
+ checkIntervalSec = rssConf.getLong(RssBaseConf.RSS_RECONFIGURE_INTERVAL_SEC);
+ }
+
+ public void startReconfigureThread() {
+ scheduledExecutorService.scheduleAtFixedRate(
+ () -> checkConfiguration(), checkIntervalSec, checkIntervalSec, TimeUnit.SECONDS);
+ }
+
+ public void stopReconfigureThread() {
+ scheduledExecutorService.shutdown();
+ }
+
+ private void checkConfiguration() {
+ long newLastModify = new File(rssConf.getString(RECONFIGURABLE_FILE_NAME, "")).lastModified();
+ if (lastModify.get() == 0) {
+ lastModify.set(newLastModify);
+ return;
+ }
+ if (newLastModify != lastModify.get()) {
Review Comment:
```suggestion
if (newLastModify > lastModify.get()) {
LOG.warn("Server detect the modification of file %s, we start to reconfigure", configuration_file_name);
```
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java:
##########
@@ -58,7 +60,7 @@ public class SimpleClusterManager implements ClusterManager {
private Map<String, Set<ServerNode>> tagToNodes = Maps.newConcurrentMap();
private AtomicLong excludeLastModify = new AtomicLong(0L);
private long heartbeatTimeout;
- private int shuffleNodesMax;
+ private AtomicInteger shuffleNodesMax;
Review Comment:
can we just use volatile?
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/AccessManager.java:
##########
@@ -107,4 +109,22 @@ public void close() throws IOException {
checker.close();
}
}
+
+ public boolean isPropertyReconfigurable(String property) {
+ for (AccessChecker checker : accessCheckers) {
+ if (checker instanceof Reconfigurable
+ && ((Reconfigurable) checker).isPropertyReconfigurable(property)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public void reconfigure(RssConf conf) {
Review Comment:
why `AccessManager` doesn't implements `Reconfigurable`?
##########
common/src/main/java/org/apache/uniffle/common/config/Reconfigurable.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * 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.uniffle.common.config;
+
+public interface Reconfigurable {
+
+ void reconfigure(RssConf conf);
Review Comment:
could we add some java doc for this and related method?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101184291
##########
common/src/main/java/org/apache/uniffle/common/config/Reconfigurable.java:
##########
@@ -0,0 +1,26 @@
+/*
+ * 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.uniffle.common.config;
+
+public interface Reconfigurable {
+
+ void reconfigure(RssConf conf);
Review Comment:
OK. I will.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101185508
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/AccessManager.java:
##########
@@ -107,4 +109,22 @@ public void close() throws IOException {
checker.close();
}
}
+
+ public boolean isPropertyReconfigurable(String property) {
+ for (AccessChecker checker : accessCheckers) {
+ if (checker instanceof Reconfigurable
+ && ((Reconfigurable) checker).isPropertyReconfigurable(property)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ public void reconfigure(RssConf conf) {
Review Comment:
My mistake. I will correct 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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101214602
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java:
##########
@@ -58,7 +60,7 @@ public class SimpleClusterManager implements ClusterManager {
private Map<String, Set<ServerNode>> tagToNodes = Maps.newConcurrentMap();
private AtomicLong excludeLastModify = new AtomicLong(0L);
private long heartbeatTimeout;
- private int shuffleNodesMax;
+ private AtomicInteger shuffleNodesMax;
Review Comment:
But I find this https://www.zhihu.com/question/329746124, I still have concern about it. I don't think atomicInteger will bring too much cost.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #572: [#410] feat: support the hot reload of coordinator's configuration
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #572:
URL: https://github.com/apache/incubator-uniffle/pull/572#discussion_r1101209602
##########
coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java:
##########
@@ -58,7 +60,7 @@ public class SimpleClusterManager implements ClusterManager {
private Map<String, Set<ServerNode>> tagToNodes = Maps.newConcurrentMap();
private AtomicLong excludeLastModify = new AtomicLong(0L);
private long heartbeatTimeout;
- private int shuffleNodesMax;
+ private AtomicInteger shuffleNodesMax;
Review Comment:
OK. I got 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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org