cpoerschke commented on pull request #479: URL: https://github.com/apache/solr/pull/479#issuecomment-1005554465
Hello @ijioio, thanks for opening this pull request! > This is how response for backup status request will looks like in case of adding `response` field: I found seeing this very helpful, thank you for including it in this PR. > * I don't think that messing with the original `Response` value is a good idea. It is a general response format used for all the core admin operations. So there may be a chance of someone depending on its content/format. Yes, if the response format were to change that would need to be clearly communicated to users e.g. via the https://github.com/apache/solr/blob/main/solr/CHANGES.txt and/or https://github.com/apache/solr/blob/main/solr/solr-ref-guide/src/major-changes-in-solr-9.adoc notes, because as you say someone might depend on its content or format. At least in the case of the `BACKUPCORE` here much of the `Response` seems more like a repeat of the request parameters rather than response information content. And via `something["Response"] instanceof String` or similar clients could differentiate between old and new format. So changing the response format could be practical then? Having `Response` and `response` elements co-exist seems to me confusing in the long-term and for new users. > * In case we use some field other than `response` we also need to update the function `aggregateResults` to support two types of response types for aggregation. In contrast, if we use the `response` field it will be compatible with current `aggregateResults` code. That's very insightful, thanks. I wonder what updating the `aggregateResults` might look like, the method has `private` visibility and so changing its signature (if necessary) would be easily possible. > Another way to solve the problem, is to put results of the operation to the `toLog` list of the response. But this implies parsing the generated string located in the `Response` field and extracting all the required fields from it within the `aggregateResults` method. Looks unnecessary complex. Yes, that looks too complex and fragile. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
