IGNITE-18121 add scale up scheduler - #1508
Conversation
| private final LogicalTopologyService logicalTopologyService; | ||
|
|
||
| private final ScheduledExecutorService executor = new ScheduledThreadPoolExecutor( | ||
| 8, |
There was a problem hiding this comment.
It's better to use an adjustable value, something like Math.min(Utils.cpus() * 3, 20);
|
|
||
| private final ScheduledExecutorService executor = new ScheduledThreadPoolExecutor( | ||
| 8, | ||
| new NamedThreadFactory("dst-zones-scheduler", LOG), |
There was a problem hiding this comment.
Let's also add node name as a prefix, e.g.
NamedThreadFactory.threadPrefix(clusterNetSvc.localConfiguration().getName(),
and extract dst-zones-scheduler to a constant.
| .forEach(zoneName -> { | ||
| int zoneId = zonesConfiguration.distributionZones().get(zoneName).zoneId().value(); | ||
|
|
||
| zonesTimers.put(zoneId, new ZoneState()); |
There was a problem hiding this comment.
And don't forget about the default zone, that is not the part of zonesConfiguration.distributionZones() but zonesConfiguration.defaultDistributionZone()
| updateMetaStorageOnZoneCreate(ctx.newValue().zoneId(), ctx.storageRevision()); | ||
| int zoneId = ctx.newValue().zoneId(); | ||
|
|
||
| ZoneState timers = new ZoneState(); |
| if (invokeResult) { | ||
| lockForZone.lock(); | ||
| try { | ||
| zoneState.nodesToAdd().clear(); |
There was a problem hiding this comment.
I don't think that it's valid. Let's imagine that while we are here, trying to add +D, new node was added to zoneState.nodesToAdd(), so it's +D,E. We should just exclude the delta we've successfully added.
There was a problem hiding this comment.
yeah, thats obviously an inattention from my side, algorithm in pseudocode has the logic that you've described
| lockForZone.unlock(); | ||
| } | ||
| } else { | ||
| return saveDataNodesToMetaStorageOnScaleUp(zoneId, revision); |
|
|
||
| if (newScaleUp != Integer.MAX_VALUE && oldScaleUp != newScaleUp) { | ||
| // It is safe to zonesTimers.get(zoneId) in term of NPE because meta storage notifications are one-threaded | ||
| zonesTimers.get(zoneId).rescheduleOngoingScaleUp(newScaleUp); |
There was a problem hiding this comment.
Why do you expect mandatory ongoing timer at this point? Let's say that that timer was already fired, should we consider it as an ongoing one, probably not.
| scaleUp.reschedule(delay); | ||
| } | ||
|
|
||
| private void rescheduleOngoingScaleUp(long delay) { |
There was a problem hiding this comment.
Provided logic with (ongoing)rescheduling is error-prone and cumbersome, please rethink and reimplement it. There are 3 possible states for timers:
- no scheduled task.
- task is scheduled but not fired yet.
- task is executing right now.
In case of 1 and 3 we should schedule new task, so it's possible for current timer to execute one right now and have one more in a pending queue.
In case of option 2 we should reschedule the task.
Aforementioned scheduling and rescheduling should be thread-safe.
| ops().yield(false) | ||
| ); | ||
|
|
||
| metaStorageManager.invoke(iifScaleUp); |
There was a problem hiding this comment.
Here and in updateMetaStorage... you are ignoring invoke result. It's probably not a good idea ;)
e3207d5 to
ab33fc8
Compare
2 participants
https://issues.apache.org/jira/browse/IGNITE-18121