igiguere commented on PR #4078: URL: https://github.com/apache/solr/pull/4078#issuecomment-3985943438
> Found a few things that very likely need changed - left comments about those inline. That should help slim things down a bit. > > In general - I'm sympathetic to the cosmetic changes you've considered in this PR but I'm having trouble reviewing the PR in detail. I just don't know our "system info" stuff to tell at a glance what's "new" and what's a refactor of the existing code. > > @igiguere would you object to our making this "just" a verbatim JAX-RS migration, and we could handle any cosmetic changes in a subsequent PR? (You could do this, if willing. Or if not, I'm happy to make the changes myself if you don't mind. I've waffled a bit on this already and don't want that to cause you additional work. Just give me a thumbs up or thumbs down and I can get started) > > In practice this would mean: > > * deleting the existing class NodeSystemInfoAPI > > * Modifying NodeSystemInfoApi to use the "/node/system" path (at least, for now) > > * Deleting `NodeSystemInfoApi.getSpecificNodeSystemInfo` (at least, for now) > > * removing the `NodeSystemInfo` wrapper-object in `NodeSystemResponse` (again - just for now. Of all your cosmetic improvements this is the one that I like the most! but it adds a ton to the diff and it'd be easier to review in a separate PR) @gerlowskija: I reverted changes to the response structure (`nodeInfo` wrapper), I removed the old V2 API `NodeSystemInfoAPI` and related unit tests. The new V2 `NodeSystemInfoApi` now uses the "/node/system" path. I deleted `NodeSystemInfoApi.getSpecificNodeSystemInfo`, along with it's implementation and unit test. I hope it helps review. I'm going through your other comments, to see if anything else still applies. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
