Centralize the concept of processors configuration #89662

fcofdez · 2022-08-26T15:14:07Z

This commit centralizes the processor count concept into the Processors class.
With this change now all the places using a processor count rely on this new class,
such as desired nodes, node.processors setting and autoscaling deciders.

Processor counts are rounded to up to 5 decimal places
Processors can be represented as doubles

Desired nodes processors were stored as floats, this poses some challenges during
upgrades as once the value is casted to a double, the precision increases and therefore
the number is not the same. In order to allow idempotent desired nodes updates after
upgrades, this commit introduces DesiredNode#equalsWithProcessorsCloseTo(DesiredNode that, double maxError)
which allows comparing two desired nodes that differ up to maxError in their processor
specification as floats.

fcofdez · 2022-08-30T00:12:46Z

@elasticmachine run elasticsearch-ci/bwc

elasticsearchmachine · 2022-08-30T07:45:40Z

Pinging @elastic/es-distributed (Team:Distributed)

elasticsearchmachine · 2022-08-30T07:45:40Z

Hi @fcofdez, I've created a changelog YAML for you.

…ch into processors-precision

henningandersen

I went through the production code and left comments.

henningandersen · 2022-08-30T08:23:12Z

server/src/main/java/org/elasticsearch/common/unit/Processors.java

+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        return builder.value(count);


Could this produce more than 5 decimals?

count is already truncated to 5 decimal places in the constructor. Additionally, there's a test (ProcessorsTests#testTruncatesAfterFiveDecimalPlaces) that checks that the string representation has up to 5 decimal places.

I tried to read Double.toString. IIUC, we would get scientific notation for numbers less than 1e-3? Maybe not a problem though, I suppose it is json compatible.

I also worry that there is some number, where even when rounded to 5 decimal places we need more decimals to represent it uniquely. I do not have an example, but did you run the test thousands of times to check?

I tried to read Double.toString. IIUC, we would get scientific notation for numbers less than 1e-3? Maybe not a problem though, I suppose it is json compatible.

According to RFC-8259 the scientific notation should be supported by any standard parser.

I also worry that there is some number, where even when rounded to 5 decimal places we need more decimals to represent it uniquely. I do not have an example, but did you run the test thousands of times to check?

I improved the test and I've run it 10K times.

henningandersen · 2022-08-30T08:38:09Z

server/src/main/java/org/elasticsearch/common/unit/Processors.java

+    }
+
+    private static boolean validNumberOfProcessors(double processors) {
+        return Double.isFinite(processors) && processors >= 0.0;


Do we need to support 0.0?

Hmm, now seeing this is indeed needed for autoscaling. Sad, I wish we could find another way there, but that is for another day.

But can we ensure that only the ZERO constant can create a 0.0 value by having a special private constructor?

Can we remove 0.0 as a valid number now?

Suggested change

return Double.isFinite(processors) && processors >= 0.0;

return Double.isFinite(processors) && processors > 0.0;

henningandersen · 2022-08-30T08:41:52Z

...a/org/elasticsearch/action/admin/cluster/desirednodes/TransportUpdateDesiredNodesAction.java

@@ -39,6 +39,7 @@

 public class TransportUpdateDesiredNodesAction extends TransportMasterNodeAction<UpdateDesiredNodesRequest, UpdateDesiredNodesResponse> {
    private static final Logger logger = LogManager.getLogger(TransportUpdateDesiredNodesAction.class);
+    private static final double MAX_DELTA_PROCESSORS = 7E-5;


This is a magic constant, how did you come to this? Would this also work for 128 processors for instance?

I run quite a few experiments in DesiredNodeTests#testFloatProcessorsConvertedToDoubleAreCloseToEqual starting from 1E-5 but that failed quite a few times. I increased it by 1E-5 at a time until the test stopped failing. I agree that's not super scientific method.

henningandersen · 2022-08-30T08:45:10Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java

+        float floatCount = (float) a.count();
+        float otherFloatCount = (float) b.count();
+        return Float.isFinite(floatCount) && Float.isFinite(otherFloatCount) && (Math.abs(floatCount - otherFloatCount) < maxError);


Can we instead use Math.max(Math.ulp(floatCount), Math.ulp(otherFloatCount)) + MIN_REPRESENTABLE_PROCESSORS as the error bound? Or has the number been rounded during x-content too?

I also wonder if we can be slightly more precise since we could know that the new number originates from a double? Might not be worth it though...

henningandersen · 2022-08-30T09:19:39Z

server/src/main/java/org/elasticsearch/monitor/os/OsInfo.java


    private final long refreshInterval;
    private final int availableProcessors;
-    private final int allocatedProcessors;
+    private final Processors allocatedProcessors;


I suppose this changes our nodes x-content output to have double processors. I wonder if we need to consider that a breaking change, since it could break downstream clients?

Good point, I'll serialize the rounded allocated processors in x-content. I wonder if we want to serialize the fractional value in an additional field.

henningandersen · 2022-08-30T10:08:21Z

server/src/main/java/org/elasticsearch/cluster/desirednodes/DesiredNodesSettingsValidator.java

@@ -92,7 +92,8 @@ private void validate(DesiredNode node) {
            int minProcessors = node.roundedDownMinProcessors();
            Integer roundedUpMaxProcessors = node.roundedUpMaxProcessors();
            int maxProcessors = roundedUpMaxProcessors == null ? minProcessors : roundedUpMaxProcessors;
-            Setting.intSetting(NODE_PROCESSORS_SETTING.getKey(), minProcessors, 1, maxProcessors, Setting.Property.NodeScope).get(settings);
+            Setting.doubleSetting(NODE_PROCESSORS_SETTING.getKey(), minProcessors, 1, maxProcessors, Setting.Property.NodeScope)


Should we lower the minValue to Double.MIN_VALUE or Processors.MIN_REPRESENTABLE_PROCESSORS?

henningandersen · 2022-08-30T10:10:11Z

server/src/main/java/org/elasticsearch/cluster/metadata/DesiredNode.java

-    private static int roundDown(float value) {
-        return Math.max(1, (int) Math.floor(value));
+    private static boolean invalidNumberOfProcessors(Processors processors) {
+        return processors != null && processors.count() <= 0;


I think this can only be true for processors.count() == 0 now, but is that even a legal case for Processors? Given that we now validate Processors separately perhaps this method is no longer needed?

henningandersen · 2022-08-30T10:28:54Z

server/src/main/java/org/elasticsearch/common/unit/Processors.java

+    }
+
+    @Nullable
+    public static Processors of(Double count) {


I wonder if we should move all Processors construction to use an of method, just to simplify and avoid mistakes? That of method could hold the validation too then, helping on the ZERO part I commented on in another comment (though a minor point).

henningandersen · 2022-08-30T10:30:34Z

...c/main/java/org/elasticsearch/xpack/autoscaling/existence/FrozenExistenceDeciderService.java

@@ -56,7 +56,7 @@ public AutoscalingDeciderResult scale(Settings configuration, AutoscalingDecider
            builder.total(MINIMUM_FROZEN_STORAGE, MINIMUM_FROZEN_MEMORY, null);
            builder.node(MINIMUM_FROZEN_STORAGE, MINIMUM_FROZEN_MEMORY, null);
        } else {
-            builder.total(0L, 0L, 0f);
+            builder.total(ByteSizeValue.ZERO, ByteSizeValue.ZERO, null);


Should we use Processors.ZERO instead of null?

henningandersen · 2022-08-30T10:32:23Z

...lugin/autoscaling/src/test/java/org/elasticsearch/xpack/autoscaling/AutoscalingTestCase.java

@@ -153,6 +154,10 @@ public static AutoscalingPolicy randomAutoscalingPolicyOfName(final String name)
        return new AutoscalingPolicy(name, randomRoles(), randomAutoscalingDeciders());
    }

+    public static Processors randomProcessors() {
+        return Processors.of(randomInt(128) + randomDouble());


nit: can we use randomDoubleBetween?

fcofdez · 2022-09-06T09:27:46Z

@henningandersen could you review this when you have the chance? Thanks!

henningandersen

LGTM.

henningandersen · 2022-09-06T11:36:03Z

server/src/main/java/org/elasticsearch/common/unit/Processors.java

+    }
+
+    private static boolean validNumberOfProcessors(double processors) {
+        return Double.isFinite(processors) && processors >= 0.0;


Can we remove 0.0 as a valid number now?

Suggested change

return Double.isFinite(processors) && processors >= 0.0;

return Double.isFinite(processors) && processors > 0.0;

henningandersen · 2022-09-06T11:40:36Z

server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodeTests.java

+    }
+
+    private double randomNumberOfProcessors() {
+        return randomDoubleBetween(Math.ulp(0.0), 512.99999999, true);


Is Math.ulp(0.0) not the same as Double.MIN_VALUE? I'd prefer to use the latter then.

henningandersen · 2022-09-06T11:42:27Z

server/src/test/java/org/elasticsearch/cluster/metadata/DesiredNodesTestCase.java

@@ -184,4 +184,8 @@ public static DiscoveryNode newDiscoveryNode(String nodeName) {
            Version.CURRENT
        );
    }
+
+    public static double randomNumberOfProcessors() {
+        return randomDoubleBetween(Math.ulp(0.0), 512.99999999, true);


Also use Double.MIN_VALUE here?

henningandersen · 2022-09-06T11:43:02Z

server/src/test/java/org/elasticsearch/common/unit/ProcessorsSerializationTests.java

+
+    @Override
+    protected Processors createTestInstance() {
+        return Processors.of(randomDoubleBetween(Math.ulp(0.0), 512.99999999, true));


Also use Double.MIN_VALUE here?

henningandersen · 2022-09-06T11:44:31Z

server/src/test/java/org/elasticsearch/common/unit/ProcessorsTests.java

+    }
+
+    public void testRounding() {
+        {


Can we add 1-2 more cases:

A number higher than 1.something, for instance 10.1.

A number that is already an integer, for instance 10.

henningandersen · 2022-09-06T11:47:21Z

server/src/test/java/org/elasticsearch/common/unit/ProcessorsTests.java

+import static org.hamcrest.Matchers.lessThanOrEqualTo;
+
+public class ProcessorsTests extends ESTestCase {
+    public void testTruncatesAfterFiveDecimalPlaces() {


For completeness, can we add a test that Processors.of(value).count() == value?

.../org/elasticsearch/xpack/autoscaling/capacity/AutoscalingCapacityWireSerializationTests.java

fcofdez · 2022-09-06T16:17:45Z

Thanks Henning!

elasticsearchmachine added the v8.5.0 label Aug 26, 2022

fcofdez force-pushed the processors-precision branch 6 times, most recently from a047e87 to e5d90ae Compare August 29, 2022 22:44

Centralize the concept of processors configuration

46ebfc1

fcofdez force-pushed the processors-precision branch from e5d90ae to 46ebfc1 Compare August 29, 2022 23:43

fcofdez added >enhancement :Distributed Coordination/Autoscaling Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. labels Aug 30, 2022

fcofdez requested a review from henningandersen August 30, 2022 07:45

fcofdez marked this pull request as ready for review August 30, 2022 07:45

fcofdez added 4 commits August 30, 2022 09:45

Update docs/changelog/89662.yaml

92cc596

Include fractional processors in the OsInfo response

2aca87a

Merge branch 'processors-precision' of github.com.:fcofdez/elasticsear…

2976786

…ch into processors-precision

Fix missing test call

45d2646

henningandersen reviewed Aug 30, 2022

View reviewed changes

henningandersen mentioned this pull request Aug 30, 2022

[ML] Add processor autoscaling decider #89645

Merged

fcofdez added 3 commits August 31, 2022 11:25

Review comments

7d1cb57

Merge remote-tracking branch 'origin/main' into processors-precision

7a22165

Revert change

b1b865a

fcofdez requested a review from henningandersen August 31, 2022 11:20

fcofdez added 2 commits September 1, 2022 16:11

Improve test

152d9b3

Merge remote-tracking branch 'origin/main' into processors-precision

b0d113f

henningandersen approved these changes Sep 6, 2022

View reviewed changes

fcofdez added 3 commits September 6, 2022 16:41

Review comments

d8ff31f

Merge remote-tracking branch 'origin/main' into processors-precision

05c3f7f

Remove dead code

53f9d49

fcofdez merged commit 284dce6 into elastic:main Sep 6, 2022

henningandersen mentioned this pull request Nov 14, 2022

Size JVM pools based on node.processors #91543

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize the concept of processors configuration #89662

Centralize the concept of processors configuration #89662

fcofdez commented Aug 26, 2022 •

edited

Loading

fcofdez commented Aug 30, 2022

elasticsearchmachine commented Aug 30, 2022

elasticsearchmachine commented Aug 30, 2022

henningandersen left a comment

henningandersen Aug 30, 2022

fcofdez Aug 31, 2022

henningandersen Aug 31, 2022

fcofdez Sep 1, 2022

henningandersen Aug 30, 2022

henningandersen Aug 30, 2022

henningandersen Sep 6, 2022

henningandersen Aug 30, 2022

fcofdez Aug 31, 2022

henningandersen Aug 30, 2022

henningandersen Aug 30, 2022

fcofdez Aug 31, 2022

henningandersen Aug 30, 2022

henningandersen Aug 30, 2022

henningandersen Aug 30, 2022

henningandersen Aug 30, 2022

henningandersen Aug 30, 2022

fcofdez commented Sep 6, 2022

henningandersen left a comment

henningandersen Sep 6, 2022

henningandersen Sep 6, 2022

henningandersen Sep 6, 2022

henningandersen Sep 6, 2022

henningandersen Sep 6, 2022

henningandersen Sep 6, 2022

fcofdez commented Sep 6, 2022

	return Double.isFinite(processors) && processors >= 0.0;
	return Double.isFinite(processors) && processors > 0.0;

Centralize the concept of processors configuration #89662

Centralize the concept of processors configuration #89662

Conversation

fcofdez commented Aug 26, 2022 • edited Loading

fcofdez commented Aug 30, 2022

elasticsearchmachine commented Aug 30, 2022

elasticsearchmachine commented Aug 30, 2022

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcofdez commented Sep 6, 2022

henningandersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcofdez commented Sep 6, 2022

fcofdez commented Aug 26, 2022 •

edited

Loading