HDDS-10292. Extract configurator for size settings from MiniOzoneCluster #6167
Conversation
…nt unit parameter; remove separation of config source/target
adoroszlai
added
test
code-cleanup
adoroszlai
requested review from
aswinshakil,
fapifta,
sadanand48 and
szetszwo
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@adoroszlai , thanks for working on this! I have to quick comments inlined.
| private OptionalLong blockSize = OptionalLong.empty(); | ||
| private OptionalInt streamBufferSize = OptionalInt.empty(); | ||
| private OptionalLong streamBufferFlushSize = OptionalLong.empty(); | ||
| private OptionalLong dataStreamBufferFlushSize = OptionalLong.empty(); | ||
| private OptionalLong dataStreamWindowSize = OptionalLong.empty(); | ||
| private OptionalLong streamBufferMaxSize = OptionalLong.empty(); | ||
| private OptionalInt dataStreamMinPacketSize = OptionalInt.empty(); |
There was a problem hiding this comment.
Let's also change the fields to not to use Optional. In general, it is not a good practice to use Optional as a field. See this comment by Brian Goetz (the main author of Java Concurrency in Practice).
You should almost never use it as a field of something or a method parameter.
| /** | ||
| * Helper for tests that want to set client stream properties. | ||
| */ | ||
| public final class ClientConfigBuilder { |
There was a problem hiding this comment.
Since this is for testing only, how about calling git ClientConfigForTesting?
There was a problem hiding this comment.
Thanks for the suggestion. I wasn't quite happy with the original name, but couldn't find a better one.
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
|
Thanks @szetszwo for the review. |
2 participants
What changes were proposed in this pull request?
Create a dedicated configurator object for setting the following properties for tests, instead of polluting
MiniOzoneCluster.Builderinterface:blockSizechunkSizedataStreamBufferFlushSizedataStreamMinPacketSizedatastreamWindowSizestreamBufferFlushSizestreamBufferMaxSizestreamBufferSizeThe first commit in the PR (284f3ec) tries to minimize change for easier review. Follow-up commits are minor additional improvements.
Moving the default values for these properties to
ozone-site.xmluncovered a problem withOzoneClientConfigconstruction, which is also fixed here (445db63). (See #6163 for similar fix in prod. code.)https://issues.apache.org/jira/browse/HDDS-10292
How was this patch tested?
CI:
https://github.com/adoroszlai/ozone/actions/runs/7782795270