[SERVER-40958] Deprecate the 'scandata' argument in 'validate' Created: 02/May/19 Updated: 29/Oct/23 Resolved: 30/May/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Storage |
| Affects Version/s: | None |
| Fix Version/s: | 4.1.14 |
| Type: | Improvement | Priority: | Minor - P4 |
| Reporter: | Gregory Wlodarek | Assignee: | Gregory Noma |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | neweng | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||
| Backport Requested: |
v4.0, v3.6
|
||||||||||||||||||||
| Sprint: | Execution Team 2019-06-03 | ||||||||||||||||||||
| Participants: | |||||||||||||||||||||
| Description |
|
Today, setting scandata to true, sets the ValidateCmdLevel to kValidateRecordStore, which is not checked against anywhere, and therefore unused. We should log a message saying that this argument is deprecated and remove it in a future release. Additionally, we can convert the 'validate' command to use IDL. |
| Comments |
| Comment by Gregory Noma [ 31/May/19 ] |
|
I think it makes sense to simply remove it from the 3.6 -> 4.2 documentation, given that it does nothing in these releases. |
| Comment by Ravind Kumar (Inactive) [ 31/May/19 ] |
|
Ah, I likely misinterpreted the if/elseif. In that case, in the short term are there objections to simply removing scandata from 3.6 -> 4.2 documentation, or simply marking it as deprecated w/ no function description? If it wasn't doing anything before, and we've deprecated it moving forward, it seems unlikely to be a breaking change for users utilizing this (again considering this is an internal-facing utility). |
| Comment by Gregory Noma [ 31/May/19 ] |
|
I don't think that scandata stomps full. If full is set to true then the value of scandata is completely ignored. If full is set to false then the value of scandata is read but never used. Thus I believe that it behaves as expected based on the value of full. |
| Comment by Ravind Kumar (Inactive) [ 31/May/19 ] |
|
I think so, especially if the option doesn't really do anything. I would argue it might be a bug, since a user could set full : true and then scandata:false and end up doing something unexpected (assuming my reading of the code is correct and setting scandata stomps the value of full. If that's a trivial fixup, at least. If it's non-trivial, I'd like to at least remove the option so users don't risk specifying it and getting unexpected results. This is an internal-only command, I don't know how often folks are actually using it, but to be safe its an easy fix. |
| Comment by Gregory Noma [ 31/May/19 ] |
|
ravind.kumar ValidateRecordStore appears to not be checked anywhere since 3.6, thus causing scandata to have no effect. Do you think it would make sense to backport this to 3.6? |
| Comment by Ravind Kumar (Inactive) [ 31/May/19 ] |
|
gregory.noma question: Looking at the code prior to 4.2, it looks like specifying validate with scandata would stomp full. I'm having trouble tracking down exactly where kValidateRecordStore is used and what it implies, but if my rough reading of the code is correct, it looks like I'll need to patch the behavior in 4.0 and possibly earlier.
|
| Comment by Githook User [ 30/May/19 ] |
|
Author: {'email': 'gregory.noma@gmail.com', 'name': 'Gregory Noma', 'username': 'gregorynoma'}Message: |