-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix incorrect logic in serverHealthInfo #3442
Conversation
It was being assumed that whole response has been received as soon as info.Version is non-empty. This is wrong as the very first response by minio contains the version. So removed the unnecessary for loop that had this check to ensure that the whole report is received properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HealthInfoHandler
uses a channel and go-routines. The healthInfo
object always seems to include the version, so this seems to be safe and do the same thing as before. No need to loop...
During reviewing, I did start to wonder the functionality of this function. It doesn't make much sense to get the health version number on its own. It looks like this method sends data in parts, so we won't get much information here. It only will return the partial information of the first go-routine that completes. Does this need more investigation?
The server returns the data in multiple iterations, but not incrementally. The same top level structure keeps getting filled with more data in each iteration (including data from previous iterations) - so what we receive in the last iteration is the complete data (first iteration will have almost no data - just two fields - deployment id and the version). I think the original reason behind this is to be able to show progress of report generation on
The report format has changed over a period of time, so the version number serves two purposes:
Here, since the purpose is to simply fetch the report and not process it in any way, the version number can be ignored, and we can also ignore initial responses from minio and wait for all data to be received before returning it to the frontend. |
It was being assumed that whole response has been received as soon as info.Version is non-empty. This is wrong as the very first response by minio contains the version. So removed the unnecessary for loop that had this check to ensure that the whole report is received properly.