You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2020/04/15 21:46:55 UTC
[GitHub] [storm] Ethanlm opened a new pull request #3249: [STORM-3624] Spawn
a SchedulerConfigRefresher thread to reload scheduler configs periodically
Ethanlm opened a new pull request #3249: [STORM-3624] Spawn a SchedulerConfigRefresher thread to reload scheduler configs periodically
URL: https://github.com/apache/storm/pull/3249
https://issues.apache.org/jira/browse/STORM-3624
This avoid race condition and makes the config loading logic cleaner.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3249:
[STORM-3624] Fix race condition when loading scheduler configs
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3249: [STORM-3624] Fix race condition when loading scheduler configs
URL: https://github.com/apache/storm/pull/3249#discussion_r409816471
##########
File path: storm-server/src/main/java/org/apache/storm/scheduler/multitenant/MultitenantScheduler.java
##########
@@ -30,20 +32,22 @@
private static final Logger LOG = LoggerFactory.getLogger(MultitenantScheduler.class);
protected IConfigLoader configLoader;
private Map<String, Object> conf;
+ private SchedulerConfigCache<Map<String, Number>> schedulerConfigCache;
@Override
public void prepare(Map<String, Object> conf, StormMetricsRegistry metricsRegistry) {
this.conf = conf;
configLoader = ConfigLoaderFactoryService.createConfigLoader(conf);
-
+ schedulerConfigCache = new SchedulerConfigCache<>(conf, this::loadConfig);
Review comment:
There are currently two schedulers (ResourceAwareScheduler, MultitenantScheduler) need to have this logic. And maybe in the future a new scheduler or other existing schedulers will want to have configs too. I keep this class to wrap the logic and to avoid repeat the code in schedulers.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3249:
[STORM-3624] Fix race condition when loading scheduler configs
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3249: [STORM-3624] Fix race condition when loading scheduler configs
URL: https://github.com/apache/storm/pull/3249#discussion_r409816471
##########
File path: storm-server/src/main/java/org/apache/storm/scheduler/multitenant/MultitenantScheduler.java
##########
@@ -30,20 +32,22 @@
private static final Logger LOG = LoggerFactory.getLogger(MultitenantScheduler.class);
protected IConfigLoader configLoader;
private Map<String, Object> conf;
+ private SchedulerConfigCache<Map<String, Number>> schedulerConfigCache;
@Override
public void prepare(Map<String, Object> conf, StormMetricsRegistry metricsRegistry) {
this.conf = conf;
configLoader = ConfigLoaderFactoryService.createConfigLoader(conf);
-
+ schedulerConfigCache = new SchedulerConfigCache<>(conf, this::loadConfig);
Review comment:
There are currently two schedulers (ResourceAwareScheduler, MultitenantScheduler) need to have this logic (caching/refreshing). And maybe in the future a new scheduler or other existing schedulers will want to have configs too. I keep this class to wrap the logic and to avoid repeat the code in schedulers.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] bipinprasad commented on issue #3249: [STORM-3624] Spawn a
SchedulerConfigRefresher thread to reload scheduler configs periodically
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on issue #3249: [STORM-3624] Spawn a SchedulerConfigRefresher thread to reload scheduler configs periodically
URL: https://github.com/apache/storm/pull/3249#issuecomment-614550574
Weird build failures - appears that simulated time is failing on some builds - e.g SlotTest.testResourcesChangedFiltered Line 744, etc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm edited a comment on issue #3249: [STORM-3624] Fix
race condition when loading scheduler configs
Posted by GitBox <gi...@apache.org>.
Ethanlm edited a comment on issue #3249: [STORM-3624] Fix race condition when loading scheduler configs
URL: https://github.com/apache/storm/pull/3249#issuecomment-614809676
Per @kishorvpatil 's suggestion, eliminated the new thread and changed to refresh every time before scheduling.
I am still keeping a separate class (SchedulerConfigCache) to deal with the cache/refresh logic to avoid code repetition in different schedulers (e.g. ResourceAwareScheduler, MultitenantScheduler)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on issue #3249: [STORM-3624] Fix race
condition when loading scheduler configs
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on issue #3249: [STORM-3624] Fix race condition when loading scheduler configs
URL: https://github.com/apache/storm/pull/3249#issuecomment-614839341
Noticed
```
[ERROR] testFIFOEvictionStrategy Time elapsed: 0.016 s <<< ERROR!
java.lang.NullPointerException
at java.util.Collections$UnmodifiableMap.<init>(Collections.java:1446)
at java.util.Collections.unmodifiableMap(Collections.java:1433)
at org.apache.storm.scheduler.resource.ResourceAwareScheduler.config(ResourceAwareScheduler.java:110)
at org.apache.storm.scheduler.resource.ResourceAwareScheduler.getUsers(ResourceAwareScheduler.java:450)
at org.apache.storm.scheduler.resource.ResourceAwareScheduler.schedule(ResourceAwareScheduler.java:118)
at org.apache.storm.scheduler.resource.strategies.priority.TestFIFOSchedulingPriorityStrategy.testFIFOEvictionStrategy(TestFIFOSchedulingPriorityStrategy.java:64)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
```
will fix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on issue #3249: [STORM-3624] Spawn a
SchedulerConfigRefresher thread to reload scheduler configs periodically
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on issue #3249: [STORM-3624] Spawn a SchedulerConfigRefresher thread to reload scheduler configs periodically
URL: https://github.com/apache/storm/pull/3249#issuecomment-614677330
Just rebased to resolve conflicts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on issue #3249: [STORM-3624] Fix race
condition when loading scheduler configs
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on issue #3249: [STORM-3624] Fix race condition when loading scheduler configs
URL: https://github.com/apache/storm/pull/3249#issuecomment-614809676
Per @kishorvpatil 's suggestion, eliminated the new thread and changed to refresh every time before scheduling.
I am still keeping a separate class (SchedulerConfigCache) to deal with the logic to avoid code repetition in different schedulers (e.g. ResourceAwareScheduler, MultitenantScheduler)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3249:
[STORM-3624] Spawn a SchedulerConfigRefresher thread to reload scheduler
configs periodically
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3249: [STORM-3624] Spawn a SchedulerConfigRefresher thread to reload scheduler configs periodically
URL: https://github.com/apache/storm/pull/3249#discussion_r409586909
##########
File path: storm-server/src/main/java/org/apache/storm/scheduler/utils/SchedulerConfigRefresher.java
##########
@@ -0,0 +1,76 @@
+/**
+ * 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.storm.scheduler.utils;
+
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.storm.DaemonConfig;
+import org.apache.storm.StormTimer;
+import org.apache.storm.utils.ObjectReader;
+import org.apache.storm.utils.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SchedulerConfigRefresher<T> {
+
+ private static final Logger LOG = LoggerFactory.getLogger(SchedulerConfigRefresher.class);
+
+ private final StormTimer configRefreshTimer;
+ private AtomicReference<T> schedulerConfigAtomicReference;
+ private final Reloadable<T> reloader;
+ private final Map<String, Object> conf;
+
+ public SchedulerConfigRefresher(Map<String, Object> conf, Reloadable<T> reloader) {
+ configRefreshTimer = new StormTimer("scheduler-config-refresher", (t, e) -> {
+ LOG.error("Error while refreshing scheduler config", e);
+ Utils.exitProcess(20, "Error while refreshing scheduler config");
Review comment:
If this thread has any errors, yes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] bipinprasad commented on a change in pull request #3249:
[STORM-3624] Fix race condition when loading scheduler configs
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3249: [STORM-3624] Fix race condition when loading scheduler configs
URL: https://github.com/apache/storm/pull/3249#discussion_r409813913
##########
File path: storm-server/src/main/java/org/apache/storm/scheduler/multitenant/MultitenantScheduler.java
##########
@@ -30,20 +32,22 @@
private static final Logger LOG = LoggerFactory.getLogger(MultitenantScheduler.class);
protected IConfigLoader configLoader;
private Map<String, Object> conf;
+ private SchedulerConfigCache<Map<String, Number>> schedulerConfigCache;
@Override
public void prepare(Map<String, Object> conf, StormMetricsRegistry metricsRegistry) {
this.conf = conf;
configLoader = ConfigLoaderFactoryService.createConfigLoader(conf);
-
+ schedulerConfigCache = new SchedulerConfigCache<>(conf, this::loadConfig);
Review comment:
Since SchedulerConfigCache is now just a wrapper around loadConfig() method, can the class be removed and loadConfig() be called directly at the beginning of schedule?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] bipinprasad commented on a change in pull request #3249:
[STORM-3624] Spawn a SchedulerConfigRefresher thread to reload scheduler
configs periodically
Posted by GitBox <gi...@apache.org>.
bipinprasad commented on a change in pull request #3249: [STORM-3624] Spawn a SchedulerConfigRefresher thread to reload scheduler configs periodically
URL: https://github.com/apache/storm/pull/3249#discussion_r409403093
##########
File path: storm-server/src/main/java/org/apache/storm/scheduler/utils/SchedulerConfigRefresher.java
##########
@@ -0,0 +1,76 @@
+/**
+ * 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.storm.scheduler.utils;
+
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.storm.DaemonConfig;
+import org.apache.storm.StormTimer;
+import org.apache.storm.utils.ObjectReader;
+import org.apache.storm.utils.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SchedulerConfigRefresher<T> {
+
+ private static final Logger LOG = LoggerFactory.getLogger(SchedulerConfigRefresher.class);
+
+ private final StormTimer configRefreshTimer;
+ private AtomicReference<T> schedulerConfigAtomicReference;
+ private final Reloadable<T> reloader;
+ private final Map<String, Object> conf;
+
+ public SchedulerConfigRefresher(Map<String, Object> conf, Reloadable<T> reloader) {
+ configRefreshTimer = new StormTimer("scheduler-config-refresher", (t, e) -> {
+ LOG.error("Error while refreshing scheduler config", e);
+ Utils.exitProcess(20, "Error while refreshing scheduler config");
Review comment:
Will this terminate the currently running process?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services