Add support for Dataproc Flexible Machine Types in compute profiles - #16204
Add support for Dataproc Flexible Machine Types in compute profiles#16204123-komal wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Dataproc Instance Flexibility Policies by adding configuration options for master and worker flex VM machine types. However, the current implementation is prone to throwing a NullPointerException at runtime because Protobuf builders do not accept null values when these optional properties are omitted. Feedback has been provided to conditionally build and safely apply these flexibility policies, as well as to support comma-separated machine types.
123-komal
changed the title
da0c03c to
c22ed75
Compare
035da77 to
03ed2d9
Compare
| } | ||
| } | ||
|
|
||
| private List<String> formatFlexMachineTypes(List<String> flexTypes, int cpus, int memoryMb) { |
There was a problem hiding this comment.
Can be simplified as:
private List<String> formatFlexMachineTypes(List<String> flexTypes, int cpus, int memoryMb) {
return flexTypes.stream()
.map(type -> getMachineType(type, cpus, memoryMb))
.collect(Collectors.toUnmodifiableList());
}There was a problem hiding this comment.
toUnmodifiableList() option was present for the given java version so I tweak it the logic that you provided too the below one
private List<String> formatFlexMachineTypes(List<String> flexTypes, int cpus, int memoryMb) {
List<String> result = flexTypes.stream()
.map(type -> getMachineType(type, cpus, memoryMb))
.collect(Collectors.toList());
return Collections.unmodifiableList(result);
}
| return Strings.isNullOrEmpty(val) | ||
| ? Collections.emptyList() | ||
| : Collections.unmodifiableList( | ||
| Splitter.on(',').trimResults().omitEmptyStrings().splitToList(val)); |
There was a problem hiding this comment.
Splitter.splitToList returns immutable list. No need to wrap it again.
private static final Splitter COMMA_SPLITTER =
Splitter.on(',').trimResults().omitEmptyStrings();
private static List<String> getStringList(Map<String, String> properties, String key) {
String val = getString(properties, key);
return Strings.isNullOrEmpty(val)
? List.of()
: COMMA_SPLITTER.splitToList(val);
}There was a problem hiding this comment.
I tried using the same method you provide, but the List.of() is not supported with the given java version. I think it was introduced in the later ones so I used the below one
private static final Splitter COMMA_SPLITTER =
Splitter.on(',').trimResults().omitEmptyStrings();
private static List<String> getStringList(Map<String, String> properties, String key) {
String val = getString(properties, key);
return Strings.isNullOrEmpty(val)
? Collections.emptyList()
: COMMA_SPLITTER.splitToList(val);
}
| "n1", | ||
| "n2", | ||
| "n2d", | ||
| "e2" |
There was a problem hiding this comment.
Should n4 be present in the list?
There was a problem hiding this comment.
I have kept the supported option same as the one with we listed in masterMachineType. But we can still n4 during runtime, which I also tested and it worked.
| Assert.assertEquals(1, apiCount.intValue()); | ||
| Assert.assertEquals(1, genericCount.intValue()); | ||
| } | ||
| @Test |
There was a problem hiding this comment.
Missing new line before this.
| * Parses a comma-separated string property into a trimmed list of strings, | ||
| * or returns an empty list if null/empty. | ||
| */ | ||
| private static List<String> getStringList(Map<String, String> properties, String key) { |
There was a problem hiding this comment.
Can we check other fields that are "widget-type": "csv"
Example : networkTags ,scopes etc..
How are they parsed ?
And if this function can be made generic to be used for other such fields?
There was a problem hiding this comment.
I checked the other "widget-type": "csv" fields (networkTags, scopes, and initActions).
networkTags is a standard CSV, and we can use the same method getStringList for it. However, scopes has domain-specific logic (appending CLOUD_PLATFORM_SCOPE and deduplicating), and initActions has its own constructor and getter lifecycle. I keep their update logic same and it's logic is different from the getStringList.
Add support for Dataproc Flexible Machine Types in compute profiles Adding Support for the Flexvm Updated Updated Updated Updated Updated Updated Updated
03ed2d9 to
22dbc60
Compare
|
| .setGceClusterConfig(clusterConfig.build()) | ||
| .setSoftwareConfig(softwareConfigBuilder); | ||
| .setEndpointConfig(EndpointConfig.newBuilder() | ||
| .setEnableHttpPortAccess(conf.isComponentGatewayEnabled()) |
There was a problem hiding this comment.
nit: fix indentation, if broken.
3 participants
Adds support for configuring flexible fallback machine types for Dataproc master and worker nodes. This allows clusters to automatically fall back to alternative machine types in priority order if the primary machine type is unavailable.
Changes
Testing