Uploaded image for project: 'Accumulo'
  1. Accumulo
  2. ACCUMULO-2401

client code cannot create an RFile without logging a warning

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.0
    • Fix Version/s: 1.5.2, 1.6.0
    • Component/s: client
    • Labels:

      Description

      Examining the output of the SimpleProxyIT, I noticed the test was always printing a message about not being able to access accumulo-site.xml. This is coming from a call to AccumuloConfiguration.getSiteConfiguration() buried down in BCFile.Writer for the encryption stuff.

      Client-side code should not be calling getSiteConfiguration().

        Issue Links

          Activity

          Hide
          ctubbsii Christopher Tubbs added a comment -

          Sorry, that should have been "shell config command", not "documentation".

          Experimental properties were hidden from the output of the shell config command, unless they were actually used, by excluding them from the DefaultConfiguration. The reason is that the semantics of "experimental" imply "optional", and "optional" implies that it could be null/unset/absent, and still work correctly. While I'm primarily concerned about prominent exposure of experimental properties through the shell and documentation, the same argument applies to anything that uses the DefaultConfiguration, because that's where the semantics of "experimental" are communicated to the user (by their omission/lack of prominence).

          Because the DefaultConfiguration conveys the semantics about which properties are prominently exposed as available, I think experimental properties should continue to be omitted from it, and we add a test to ensure that the defaults for these are null and excluded. The code that depends on them being otherwise should be changed to handle them being null, or they should not be considered experimental. Alternatively, we'd have to find a better way to convey that certain properties are experimental, in spite of being so prominently exposed (and I can't think of a way to programatically communicate that, other than through omission; I think documentation that says "don't use this unless you know what you're doing; this is experimental and subject to change" is insufficient, as it will largely be ignored... if it's there, people will use it and depend on it).

          The code change, for reference:

          git log -n1 -p -U5 f2de427f -- core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java

          I made ACCUMULO-2460, for follow-on.

          Show
          ctubbsii Christopher Tubbs added a comment - Sorry, that should have been "shell config command", not "documentation". Experimental properties were hidden from the output of the shell config command, unless they were actually used, by excluding them from the DefaultConfiguration. The reason is that the semantics of "experimental" imply "optional", and "optional" implies that it could be null/unset/absent, and still work correctly. While I'm primarily concerned about prominent exposure of experimental properties through the shell and documentation, the same argument applies to anything that uses the DefaultConfiguration, because that's where the semantics of "experimental" are communicated to the user (by their omission/lack of prominence). Because the DefaultConfiguration conveys the semantics about which properties are prominently exposed as available, I think experimental properties should continue to be omitted from it, and we add a test to ensure that the defaults for these are null and excluded. The code that depends on them being otherwise should be changed to handle them being null, or they should not be considered experimental. Alternatively, we'd have to find a better way to convey that certain properties are experimental, in spite of being so prominently exposed (and I can't think of a way to programatically communicate that, other than through omission; I think documentation that says "don't use this unless you know what you're doing; this is experimental and subject to change" is insufficient, as it will largely be ignored... if it's there, people will use it and depend on it). The code change, for reference: git log -n1 -p -U5 f2de427f -- core/src/main/java/org/apache/accumulo/core/conf/DefaultConfiguration.java I made ACCUMULO-2460 , for follow-on.
          Hide
          ecn Eric Newton added a comment -

          It wasn't deliberate. Since I moved the reference to SiteConfiguration out of rfile, I had to pass a configuration in to lots of places that made rfiles. I passed in DefaultConfiguration. One of the places that uses experimental properties was... rfile, so I needed to have them in DefaultConfiguration.

          The document generator seems to ignore them. Are you talking about the javadocs?

          Show
          ecn Eric Newton added a comment - It wasn't deliberate. Since I moved the reference to SiteConfiguration out of rfile, I had to pass a configuration in to lots of places that made rfiles. I passed in DefaultConfiguration. One of the places that uses experimental properties was... rfile, so I needed to have them in DefaultConfiguration. The document generator seems to ignore them. Are you talking about the javadocs?
          Hide
          ctubbsii Christopher Tubbs added a comment -

          Eric Newton Any reason why this patch exposed experimental properties in the documentation without labeling them as experimental?

          Show
          ctubbsii Christopher Tubbs added a comment - Eric Newton Any reason why this patch exposed experimental properties in the documentation without labeling them as experimental?
          Hide
          ctubbsii Christopher Tubbs added a comment -

          I'm not sure I quite understand the justification for adding experimental properties to the DefaultConfiguration, where they were previously excluded.

          Show
          ctubbsii Christopher Tubbs added a comment - I'm not sure I quite understand the justification for adding experimental properties to the DefaultConfiguration, where they were previously excluded.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 840dba269266e4895a12ca2074d36e356076e656 in accumulo's branch refs/heads/master from Eric Newton
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=840dba2 ]

          ACCUMULO-2401 finish removing hacky site-configuration manipulation

          Show
          jira-bot ASF subversion and git services added a comment - Commit 840dba269266e4895a12ca2074d36e356076e656 in accumulo's branch refs/heads/master from Eric Newton [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=840dba2 ] ACCUMULO-2401 finish removing hacky site-configuration manipulation
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f2de427f74a8a6ffa05c6209e7161e7349458241 in accumulo's branch refs/heads/master from Eric Newton
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f2de427 ]

          ACCUMULO-2401
          removed many calls to deprecated AccumuloConfiguration.getSiteConfiguration()
          fixed hacky crypto tests that messed with the site configuration

          Show
          jira-bot ASF subversion and git services added a comment - Commit f2de427f74a8a6ffa05c6209e7161e7349458241 in accumulo's branch refs/heads/master from Eric Newton [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f2de427 ] ACCUMULO-2401 removed many calls to deprecated AccumuloConfiguration.getSiteConfiguration() fixed hacky crypto tests that messed with the site configuration
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 840dba269266e4895a12ca2074d36e356076e656 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=840dba2 ]

          ACCUMULO-2401 finish removing hacky site-configuration manipulation

          Show
          jira-bot ASF subversion and git services added a comment - Commit 840dba269266e4895a12ca2074d36e356076e656 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Eric Newton [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=840dba2 ] ACCUMULO-2401 finish removing hacky site-configuration manipulation
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f2de427f74a8a6ffa05c6209e7161e7349458241 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
          [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f2de427 ]

          ACCUMULO-2401
          removed many calls to deprecated AccumuloConfiguration.getSiteConfiguration()
          fixed hacky crypto tests that messed with the site configuration

          Show
          jira-bot ASF subversion and git services added a comment - Commit f2de427f74a8a6ffa05c6209e7161e7349458241 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from Eric Newton [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=f2de427 ] ACCUMULO-2401 removed many calls to deprecated AccumuloConfiguration.getSiteConfiguration() fixed hacky crypto tests that messed with the site configuration

            People

            • Assignee:
              ecn Eric Newton
              Reporter:
              ecn Eric Newton
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development