Improve DistributedQueryRunner.Builder #17104
Conversation
ssheikin
force-pushed
the
ssheikin/35/trino/DistributedQueryRunner.builder
branch
from
72c9341 to
12b1416
Compare
...va/io/trino/faulttolerant/iceberg/TestIcebergParquetFaultTolerantExecutionConnectorTest.java
Outdated
Show resolved
Hide resolved
| @@ -616,7 +616,7 @@ private static void closeUnchecked(AutoCloseable closeable) | |||
| { | |||
| private Session defaultSession; | |||
| private int nodeCount = 3; | |||
| private Map<String, String> extraProperties = new HashMap<>(); | |||
| private Map<String, String> extraProperties = ImmutableMap.of(); | |||
There was a problem hiding this comment.
I do not get this change. What was wrong with previous appraoch?
There was a problem hiding this comment.
HashMap.put/putAll could potentially silently overwrite existing value. This is not a case with ImmutableMap
There was a problem hiding this comment.
Oh I see. Just do putIfAbsent and assert it returns null. Code will be much simpler.
There was a problem hiding this comment.
Despite
Code will be much simpler.
This is a step backward from Immutable collections, which relaxes the code style - opens a possibility to override properties without intention. I'd prefer to left it as it is.
There was a problem hiding this comment.
I do not see it as such. It is not that ImmutableMap prevents you from overriding properties, but the logic in addProperties/addProperty.
Yet, whatever I will not fight over it.
There was a problem hiding this comment.
I agree with @losipiuk. Even if you have ImmutableMap you're still using a builder to construct it in those methods and anyone can override there if needed.
The safety comes from the fact that the only way to set these properties is via addProperties/addProperty and as long as those methods prevent overriding there's no possibility of overriding happening by mistake.
I also don't think it's worth arguing over even though the code looks much simpler and self-documenting with the change applied.
According to javadoc `setSingleCoordinatorProperty` /** * Sets coordinator properties being equal to a map containing given key and * value. * Note, that calling this method OVERWRITES previously set property values. * As a result, it should only be used when only one coordinator property needs * to be set. */ So it SILENTLY overwrites previous value of `coordinatorProperties`. So far all invocations of `createDockerizedDeltaLakeQueryRunner` were receiving at most single value, so in practice it never occurred. Also, most probably, `setSingleCoordinatorProperty` was a handy method to set only one property to `TpchQueryRunnerBuilder`. In it's turn `TpchQueryRunnerBuilder` does not manipulate with coordinator properties, so the guarantee of the only one single property is not needed. Later, if needed, the similar `addCoordinatorProperty` handy method may be added.
ssheikin
force-pushed
the
ssheikin/35/trino/DistributedQueryRunner.builder
branch
from
12b1416 to
fecb124
Compare
ssheikin
force-pushed
the
ssheikin/35/trino/DistributedQueryRunner.builder
branch
from
fecb124 to
1859cd4
Compare
|
@hashhar opinion here? |
Description
Additional context and related issues
Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: