HDDS-11963. Unify client version and layout version under one interface to accomodate in the request validator framework#7598
Conversation
…ce to accomodate in the request validator framework Change-Id: I4c1844a1dfb6127a73b46a92933db33b09c6b06e
Change-Id: I63cddb955ea6178cd489c2a83922576e8d2ea5eb
|
Please wait for clean CI run in fork before opening PR. |
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @swamirishi for continuing work on the request validator.
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("layoutValues") |
There was a problem hiding this comment.
Use @EnumSource for enums.
| @ParameterizedTest | ||
| @MethodSource("clientVersionValues") | ||
| void testClientVersionExtractor(ClientVersion expectedClientVersion, int clientVersion) { | ||
| OMRequest request = mock(OMRequest.class); | ||
| when(request.getVersion()).thenReturn(clientVersion); | ||
| Version version = VersionExtractor.CLIENT_VERSION_EXTRACTOR.extractVersion(request, null); | ||
| assertEquals(expectedClientVersion, version); | ||
| assertEquals(ClientVersion.class, VersionExtractor.CLIENT_VERSION_EXTRACTOR.getVersionClass()); | ||
| } |
There was a problem hiding this comment.
Split to two test cases:
- finds existing version: parameterize with
@EnumSource, exlucdeFUTURE_VERSION, getClientVersion.version()in the test case - versions larger than latest existing are mapped to
FUTURE_VERSION: parameterize with@ValueSourceforint delta
| private static Stream<Arguments> clientVersionValues() { | ||
| return Stream.of( | ||
| Arrays.stream(ClientVersion.values()) | ||
| .map(clientVersion -> Arguments.of(clientVersion, clientVersion.version())), | ||
| IntStream.range(1, 10) | ||
| .mapToObj(delta -> Arguments.of(ClientVersion.FUTURE_VERSION, ClientVersion.CURRENT_VERSION + delta)) | ||
| ).flatMap(Function.identity()); | ||
| } |
There was a problem hiding this comment.
Unnecessary complexity for simple test cases.
There was a problem hiding this comment.
addressed the review comments to simplify
| /** | ||
| * Base class defining the version in the entire system. | ||
| */ | ||
| public interface Version { |
There was a problem hiding this comment.
Versioned may be better for an interface (like Cloneable, etc.).
Change-Id: I11bff280f429d2eb64f8828887f7349e169e938f
Change-Id: I9ebc447eb9b60caa69e140b06fe899c58dd712a3
adoroszlai
left a comment
adoroszlai
left a comment
There was a problem hiding this comment.
Thanks @swamirishi for improving the patch, LGTM.
I have two more nits. They can be addressed in follow-up PR you'll be working on for the request validator, unless someone else requests other changes.
| ClientVersion[] versions = ClientVersion.values(); | ||
| return versions[versions.length - 2]; | ||
| return Arrays.stream(ClientVersion.values()) | ||
| .max(Comparator.comparingInt(ComponentVersion::toProtoValue)).orElse(null); |
There was a problem hiding this comment.
nit:
| .max(Comparator.comparingInt(ComponentVersion::toProtoValue)).orElse(null); | |
| .max(Comparator.comparingInt(ClientVersion::toProtoValue)).orElseThrow(); |
| when(request.getVersion()).thenReturn(ClientVersion.CURRENT_VERSION + futureVersion); | ||
| Versioned version = VersionExtractor.CLIENT_VERSION_EXTRACTOR.extractVersion(request, null); | ||
| assertEquals(ClientVersion.FUTURE_VERSION, version); | ||
| assertEquals(ClientVersion.class, VersionExtractor.CLIENT_VERSION_EXTRACTOR.getVersionClass()); |
There was a problem hiding this comment.
nit: move assertion about classes to separate test case
sumitagrawl
left a comment
sumitagrawl
left a comment
There was a problem hiding this comment.
LGTM, part of #6932 as breakdown.
|
Thanks @swamirishi for the patch, @sumitagrawl for the review. |
3 participants
What changes were proposed in this pull request?
Currently LayoutVersion & ClientVersion classes don't have a common base class even though they represent some sort version in the overall system. We should unify both these classes under one single interface which would help us perform some set of operations on the common base class in the request validator framework.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11963
How was this patch tested?
Added new unit tests & existing unit tests