Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

cli: Support Setting old Volume option names #816

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cli: Support Setting old Volume option names #816

wants to merge 1 commit into from

Conversation

aravindavk
Copy link
Member

Fixes: #739
Signed-off-by: Aravinda VK [email protected]

@aravindavk aravindavk requested review from kshlm and prashanthpai May 29, 2018 09:33
@aravindavk
Copy link
Member Author

legacy-volopts.go file generated by parsing glusterd-volume-set.c file. Script used is available here, not added to source tree since this is required only once.

https://gist.github.com/aravindavk/672146cd5b5d050309a78c948f79dc5c

@aravindavk
Copy link
Member Author

@amarts With this patch, volume set works with old option names. But Volinfo will show new names only.

@amarts
Copy link
Member

amarts commented May 29, 2018

With this patch, volume set works with old option names. But Volinfo will show new names only.

Yes, this is a good start. Lets see with this, how many regressions work, or fails. I am thinking we should do a wrapper function which can deal with compatibility in regression itself.

@prashanthpai
Copy link
Contributor

@amarts With this patch, volume set works with old option names. But Volinfo will show new names only.

We can do reverse mapping of responses back to old names as regression tests look for old names. @aravindavk Is that planned ?

This approach looks good to me as long as it's primary purpose is to re-use existing regression tests. For users, we should strongly encourage using the new names and deprecate the old ones gradually. Please add a deprecated warning when old option names are used.

@atinmu
Copy link
Contributor

atinmu commented Jun 13, 2018 via email

@aravindavk
Copy link
Member Author

We can do reverse mapping of responses back to old names as regression tests look for old names. @aravindavk Is that planned ?

This approach looks good to me as long as it's primary purpose is to re-use existing regression tests. For users, we should strongly encourage using the new names and deprecate the old ones gradually. Please add a deprecated warning when old option names are used.

I started this effort to add backward compatibility for Volume options, but now I realized a problem with new names. For example, Self heal daemon is common for disperse/replica, these options are set using replicate.<optname>, but this will add confusion to users.

I have some unanswered questions to proceed on this PR, we can discuss that next week.

@atinmu
Copy link
Contributor

atinmu commented Sep 14, 2018

Would this be needed for GCS? I guess the priority can be low as many of the option settings will be done through storage class?

@prashanthpai
Copy link
Contributor

Would this be needed for GCS? I guess the priority can be low as many of the option settings will be done through storage class?

Ack. GCS is a limited and opinionated deployment.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants