cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rafaelweingartner <...@git.apache.org>
Subject [GitHub] cloudstack pull request: CLOUDSTACK-9252: Support configurable NFS...
Date Tue, 26 Jan 2016 16:05:42 GMT
Github user rafaelweingartner commented on the pull request:

    https://github.com/apache/cloudstack/pull/1361#issuecomment-175093081
  
    Ok, I understood that you want to maintain compatibility. 
    What I do not understand is that: If there is a problem with some NFS (they do not support
version discovery), why aren’t you always using the “mountService.getMountPoint” passing
the NFS version? Most of the times you just send a null, only at the SSVM code you are using
the version; wouldn’t this cause problems in some other piece of the code base such as the
one you found out?
    You already have the method “getNfsVersion” that will return a null if the version
is not configured; I do not see a good reason not to use it; this way we would have a more
concise code.
    
    Additionally, you have duplicated the code of “getNfsVersion” in two or three places,
I believe we should avoid those duplicated code.
    
    I would also add a Java doc to the “getNfsVersion”, stating what it does, what parameters
it looks for, what happens if it does not find the parameter and where (in the DB) those parameters
are stored.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message