Refactor Archiver Client for large requests#196
Refactor Archiver Client for large requests#196jacomago wants to merge 13 commits intoChannelFinder:masterfrom
Conversation
|
This does sound like a good codeathon task |
jacomago
force-pushed
the
archclient-large-input
branch
from
f0f092d to
1edb6fe
Compare
jacomago
force-pushed
the
archclient-large-input
branch
from
1edb6fe to
e9679d1
Compare
jacomago
requested review from
anderslindho,
domonkos-ess,
georgweiss,
imretoth-ess,
ralphlange,
shroffk,
simon-ess and
tynanford
| @@ -19,10 +19,12 @@ | |||
| import org.phoebus.channelfinder.service.model.archiver.aa.ArchiveAction; | |||
| import org.phoebus.channelfinder.service.model.archiver.aa.ArchivePVOptions; | |||
| import org.springframework.beans.factory.annotation.Value; | |||
| import org.springframework.core.ParameterizedTypeReference; | |||
There was a problem hiding this comment.
Commit msg Use flux to stream large input does not explain why
| private List<String> postSupportArchivers; | ||
|
|
||
| private static WebClient webClient() { | ||
| final int size = (int) DataSize.ofMegabytes(16).toBytes(); |
There was a problem hiding this comment.
- where we sure this was necessary
- is 16 MB appropriate? from memory (having looked at other apps and discussion online) this seems like a lot
| List<String> failedPvs = new ArrayList<>(); | ||
| for (int i = 0; i < response.size(); i++) { | ||
| Map<String, String> result = response.get(i); | ||
| String pv = (i < pvs.size()) ? pvs.get(i) : "UNKNOWN_PV"; |
There was a problem hiding this comment.
Hmmm, what are we hedging against here, where response.size > pvs.size?
| } | ||
|
|
||
| @Test | ||
| void testGetStatusesInvalidResponse() { |
There was a problem hiding this comment.
Nitpick, but test naming is a bit inconsistent: for the previous two you only described action, where you here now also describe the consequence
| @@ -47,6 +47,7 @@ public class ArchiverService { | |||
| private static final String PV_NAME_KEY = "pvName"; | |||
| private static final String STATUS_KEY = "status"; | |||
| private static final String STATUS_OK = "ok"; | |||
| private static final String STATUS_ARCHIVE_SUBMITTED = "Archive request submitted"; | |||
| private static final String MGMT_VERSION_KEY = "mgmt_version"; | |||
| private static final MediaType CONTENT_TYPE = MediaType.APPLICATION_JSON; | |||
There was a problem hiding this comment.
Mix of different types of entities (info msg vs "enum" str equivalents vs (seemingly unnecessary) rebinding) - should maybe consider future change 🔧
| RecordedRequest request = mockWebServer.takeRequest(); | ||
| if ("test-archiver".equals(archiverAlias)) { | ||
| assertEquals("POST", request.getMethod()); | ||
| assertEquals("//mgmt/bpl/getPVStatus", request.getPath()); |
There was a problem hiding this comment.
Two leading slashes correct? Note you did this in multiple places
| // Empty archiver tagged | ||
| continue; | ||
| } | ||
| String version = archiverService.getVersion(aa.getValue()); |
There was a problem hiding this comment.
Nitpick, but for this commit you write in body
Now we explicitly configure it instead.
which makes it sound like this is a change that you introduce here in this commit
|
|
||
| @Autowired private final ArchiverClient archiverClient = new ArchiverClient(); | ||
| @Autowired private final ArchiverService archiverService = new ArchiverService(); | ||
|
|
There was a problem hiding this comment.
Not claiming I'm a Spring guru, but should Autowired beans be instantiated in config classes?
To me Autowired together with instantiation seems a bit confusing, but maybe I'm missing something?
| import java.util.List; | ||
| import java.util.Map; | ||
| import okhttp3.mockwebserver.MockResponse; | ||
| import okhttp3.mockwebserver.MockWebServer; |
There was a problem hiding this comment.
Spring Boot Test comes with APIs which are easy to use to mock a server, eliminating dependency to okhttp. Olog service test classes can be used for inspiration.
georgweiss
left a comment
georgweiss
left a comment
There was a problem hiding this comment.
Had some Spring Boot related comments....
This was used to distinguish support for getting status via post or query Now (from an earlier merge) we explicitly configure it instead.
Use bodyToFlux Add throwing of ArchiverServiceException Add validation of action response Add unit test for submitAction and other ArchiverService methods
Instead of the mockresponses
jacomago
force-pushed
the
archclient-large-input
branch
from
e9679d1 to
22427d3
Compare
|
4 participants
Refactored the Archiver Client to make sure it can stream large requests.
Also refactored to make it more easily unit testable, realised all the tests were mostly for the processor rather than on the client.
I added some examples for test cases I generated from a test archiver.
Added some more validation of the responses.
For future I'd like to use openapi definition from the archiver to generate a client, maybe that's a good codathon ticket.