Add verbose parameter to remove-disks endpoint valid parameter list

[ShubhamRwt]

Description

This PR resolves #2105. This PR adds verbose and skip_hard_goal_check parameter to the valid parameters list of remove-disks endpoint.

[mhratson]

Those parameters aren't being used for anything, what's the purpose of adding them?
So the request doesn't error?

[ShubhamRwt]

Yes @mhratson, so that it dosn't fail

[mhratson]

so that it dosn't fail

Sorry, but that failure, as for now, is by design. Making this particular change makes very little sense, as client should be able to handle this error without resorting to inconsistent API changes.

Why are you expecting it not to fail?

[kyguy]

Sorry, but that failure, as for now, is by design. Making this particular change makes very little sense, as client should be able to handle this error without resorting to inconsistent API changes.

I understand that these parameters have no function for this particular endpoint at this time but I wouldn't say these changes are inconsistent API changes. Most if not all the other endpoints have a verbose parameter, I don't think it is unreasonable for a user to expect them.

Of course, a client could handle the error, but I believe a small change like this would be worth the gain of making the API more user-friendly!

Why are you expecting it not to fail?

From what I understand, Shubham has existing client code that works with similar endpoints but fails with this particular endpoint due to the parameter differences.

[ShubhamRwt]

Yes @kyguy. Also When we have verbose as a parameter in our requests through we are able to get the loadBeforeOptimization which is a very beneficial thing so that we can we can compare it loadAfterOptimization. But without verbose I see the loadBeforeOptimization field doesn't show up.

[ShubhamRwt]

Hi @mhratson, just wanted to follow up on this issue, Does the above reason sounds suffice to get these changes merged. I mean when we move the data etween disks it can cause the broker load to change so I guess it would be good to have both loadBeforeOptimization and loadAfterOptimization field?

[mhratson]

but I wouldn't say these changes are inconsistent API changes

Inconsistent meaning that some endpoints accept verbose and some don't. It's already the case, but the contract is that invalid parameters cause error. Clients may relay on this behavior.
I think it's a good discussion but i'd be cautious changing API behavior quietly without at least minor version bump.

Of course, a client could handle the error, but I believe a small change like this would be worth the gain of making the API more user-friendly!

Depending how one looks at it: user using verbose parameter expecting additional output and getting nothing "verbose" in response is confusing user experience.

My opinion, to be consistent verbose has to be

1. handled by all endpoints
2. have actually verbose output

I think as is, this PR makes 2 changes that are unused, while one can argue for verbose, the skip_hard_goal_check is misleading. It bloats the API and has no function.

To reiterate:

• making it convenient for once client
• while potentially breaking behaviour for other clients

I think it's a good opportunity to revisit the current API and make consistent changes across the board not just cherry-picking for a single client.

[ShubhamRwt]

@mhratson Hi, we can probably skip the skip_hard_goal_check parameter but I think verbose is still something we are interested in. I am not 100% sure but wouldn't the loadbeforeOptimization be different then the loadAfterOptimization whe will move data between the disks? In such cases I do think that verbose parameter is required since it provides you with the loadBeforeOptimization

[ShubhamRwt]

Hi @mhratson, based on the above suggestion I went through all the POST endpoints and I guess we already have verbose enabled for the ones where we require it and disabled at the places where it is not required. Testing the verbose parameter for the remove-disks endpoint, I do see it can help us when we want to compare the before/after load. As you can see here -> https://gist.github.com/ShubhamRwt/c12fa47d36d53a0cba3473667020319f, I can see that the DISK_CAP(MB) and the FOLLOWER_NW_IN(KB/s) has changed when I tried to move the disks. I think it would be good to add verbose. Regarding the skip_hard_goal_check I don't think its needed