milleruntime commented on pull request #1968: URL: https://github.com/apache/accumulo/pull/1968#issuecomment-805867495
> instance.crypto.opts.table.t1.SOMEKEY=SOMEVALUE I don't think this will work with the way Accumulo handles table properties differently. Also aren't `instance` properties used to determine the hash for a cluster? It would also be confusing because it breaks the delineations set by the prefix properties. > With both of these, the main point is that there is only ever a single crypto service for the system. However, the functionality of that service is capable of providing crypto services for multiple tables and/or the WAL. Your change just relocates the implementation into inner classes, but they are still effectively separate classes that are configured separately, meaning separate authorities for managing the file-based crypto. Yes but the crypto implementation for Table class won't work with WALs, due to the type of encryption. I was trying to keep them separate enough to make this clear that not all types of crypto will work with WALs vs Tables but keep them together enough to implement the same interfaces. I believe what you want is what we have now, passing in the environment to the impl and letting it decided Table vs WAL. The problem is Accumulo needs to know whether the CryptoService is being used per table or tserver (WAL). Otherwise it gets very confusing looking at the code and trying to decipher the purpose of the CryptoService. Some of this can be derived from the code, for example within RFile or BCFile is obvious. But other parts are not, like the Tserver or FileUtilties. We can make sure to name variables based on use but the CryptoServiceFactory is a static utility. -- 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: [email protected]
