accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ctubbsii <...@git.apache.org>
Subject [GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Date Fri, 10 Mar 2017 18:22:23 GMT
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/228#discussion_r105457380
  
    --- Diff: minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
    @@ -149,9 +147,8 @@ String getJarFromClass(Class<?> clz) {
     
       @Override
       public void adminStopAll() throws IOException {
    -    File confDir = getConfDir();
    -    String master = getHosts(new File(confDir, "masters")).get(0);
    -    String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR + serverAccumuloConfDir,
accumuloPath, Admin.class.getName(), "stopAll"};
    +    String master = getHosts(MASTER_HOSTS_FILE).get(0);
    +    String[] cmd = new String[] {SUDO_CMD, "-u", user, serverEnv, accumuloPath, Admin.class.getName(),
"stopAll"};
    --- End diff --
    
    I wasn't assuming anything. I was observing my own ignorance about the reasons why this
was being done. In any case, you answered the question I should have asked, instead ("Why
is this being done?").
    
    Your response indicates that the reason this is being done is so that this test framework
can switch users to control an external running Accumulo instance, running as a specific user,
assuming `sudo` is configured in a very particular way to allow running the necessary executables
without a tty or authentication.
    
    I now understand why it uses `sudo`, but its inclusion suggests that this code exists
not to test the upstream Accumulo project, but to test specific downstream products. That
makes me a bit uncomfortable because I don't like the idea of the upstream project being burdened
with the maintenance of tests for downstream vendors. However, I can also see it the other
way around: that the upstream project has an interest in providing some test code to ensure
any downstream packaging meets some minimum standards of quality so that Accumulo. So, I'm
not necessarily going to argue we should remove it.
    
    I would, however, argue that we "genericize" it a bit, so we don't have to directly support
the use of privilege escalation features of an operating system for our tests. We can support
that indirectly by allowing the user to supply the launch command. The direct use of `sudo`
is mostly what makes me uncomfortable. I've seen a lot of open source projects code, and this
is the first time I've ever seen that used in any project's test suite that wasn't `sudo`
itself.


---
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