[GitHub] [accumulo] ctubbsii commented on issue #1428: Make MiniAccumuloCluster default to using RawLocalFileSystem when not using MiniDFSCluster

[GitBox <gi...@apache.org>]

ctubbsii commented on issue #1428: Make MiniAccumuloCluster default to using RawLocalFileSystem when not using MiniDFSCluster
URL: https://github.com/apache/accumulo/issues/1428#issuecomment-593629267
 
 
   @ron-rico You shouldn't have to touch any code in the `test/` module to implement the change, only in the `minicluster` module. After doing so, it may be possible to remove some redundant configuration that sets it to `RawLocalFileSystem` in the `test/` module, but it shouldn't be necessary to implement.
   
   That said, this one might be more complicated and annoying that I had realized at first:
   
   The constructor for `MiniAccumuloClusterImpl` takes a `MiniAccumuloConfigImpl`. In there, there is setup of config files, depending on `.useMiniDFS()`. I had initially thought it would be simple to add in the `fs.file.impl` property (similar to @jzgithub1 said above) if the user hadn't already overridden it.
   
   However, I now realize that the way we create the Hadoop configuration, override the config files with additional properties, and retrieve the FileSystem for use by callers of MiniAccumuloCluster, is done inconsistently at various points in time, and this isn't as simple as I had initially thought.
   
   Specifically, in `MiniAccumuloClusterImpl`'s constructor, it doesn't even appear to write out a config file at all for the relevant case (when `!useMiniDFS() && !useExistingInstance()`), and when `getFileSystem()` is called, it doesn't use the config files at all, but uses the value of `dfsUri` variable with a `new Configuration(false)` (a new empty configuration), which doesn't seem right.
   
   Additionally, when certain properties are overridden, such as in `ConfigurableMacBase`, it does so by taking the current contents of the config file (if it exists) and clobbers the explicitly set test properties if they have the same name (that's how `.addResource()` works), rather than the other way around.
   
   So, the code related to this issue is a lot more complicated (and convoluted) than I first believed. It's probably that way to support the various use cases for `MiniAccumuloCluster` that have evolved over time, but it makes this task harder.
   
   If you'd like to continue trying to implement this one, please feel free to do so... but I'm going to go ahead and remove the clearly incorrect "good first issue" label. :smiley_cat:

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services