apache
HDDS-6964. [Snapshot] Split out shared "Path based access" code from OM.
What changes were proposed in this pull request?
This PR refactors some of the OzoneManager code to make it available for use by the upcoming Om Snapshotting feature.
OmMReader class
Reading the metadata for OM snapshots is quite similiar to reading the metadata for the active fs. The main difference is a different RocksDb instance is used by the OmMetadataManager, KeyManager, etc. The code that reads that metadata is currently in the OzoneManager class.
This PR refactors that code and moves it into a new class called the OmMReader, (Ozone Metadata Reader.) To replace the code removed from the ozoneManager, an OmMReader will be constructed using the RocksDb for the active fs. For snapshots, the OmMReader will be constructed with the rocksDb instance corresponding to that snapshot.
Metrics
The code in OmMReader makes extensive use of the OMMetrics class. Snapshotting will need its own version so I've created a new interface that correspond to it: OmMReaderMetrics. This interface is used in the OmMReader code so that the OzoneManager and Snapshotting can each have their own. (They each pass their own version into the OmMReader.)
Removal of obsolete IOzoneAcl code
In the process of doing the refactoring, I discovered that much of the IOzoneAcl interface is no longer in use. So I removed those methods, (and fixed the few tests that still invoked them.)
These are the methods removed
KeyManagerImpl changes
In addition to the IOzoneAcl deletion mentioned above, I also had to change the KeyManagerImpl to not read the metadataManager through the ozoneManager.
In a few places, it was unnecessarily reading metadata like so: "ozoneManager.getMetadataManager()" instead of using the metadataManager field initialized by its constructor.
That would prevent the snapshot rocksDb instance from being read so I switched those to use the metadataManager field.
Initialization of the OmMReader field in OzoneManager
After reviewing the code, I feel the correct place to init the OmMReader is in the instantiateServices() method here.
The KeyManager and others are set there, and the OmMReader should be reset anytime that method is called and those are reset.
metrics field
However, the omMReader needs the omMetrics which is initialized after instantiateServices() is called in the ozone manager constructor.
To work around that, I moved the OMMetrics.create() to before instantiateServices() is called in the constructor:
+ metrics = OMMetrics.create();
instantiateServices(false);
omRpcAddress field
The omRpcAddress has the same problem but I was hesitant to move that code, so omMReader doesn't store it as a field. It just accesses it through the om, like so: "ozoneManager.getOmRpcServerAddr()"
volatile fields
The new omMReader field in the OzoneManager is not final, and will be changed each time instantiateServices() is called, (as it should.)
But the OzoneManager is multithreaded code, and, IMHO, such fields should be defined "volatile". But the keyManager, volumeManager fields etc are also changed by instantiateServices(), and they are not defined to be "volatile". That seems like a bug to me, but perhaps it was intentially done for performance reasons. In any case, I have not made the omMReader field volatile for now.
Let me know if you think that is correct.
Reviewing the changes
There are lot of changes in this PR but most are very straightforward. The only part that is hard to review is the code that is moved from the OzoneManager to the OmMReader and the only reason that is hard is because the diff tool isn't smart enough to find the changes in the new file.
So I created a python script that uses string matching to generate a version of OmMReader that has the original unchanged OzoneManager methods pasted in. You can see the diffs here
As you'll see, these are the main things changed, in the code moved to the OmMReader:
- A few private methods were made package-private.
- The "LOG" and "AUDIT" field names were changed to "log" and "audit". (Since they are no longer static final, checkstyle won't let them be all caps.)
- Changed the omRpcAddress access to "ozoneManager.getOmRpcServerAddr()" as mentioned above.
- Changed the call to resolveBucketLink() to ozoneManager.resolveBucketLink().
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6964
How was this patch tested?
All tests continue to work.
In my own repo, I've added some code that excercises the OmMReader to read snapshots. That will be part of the next PR.
A flaky test, unrelated to snapshotting, was preventing the tests from passing since I did the rebase. I debugged it and added the fix to this pr, (just so I could get the tests to pass.) I describe the problem here
This is the fix
For some reason, this race condition, (which is in master,) happens more frequently in the snapshotting branch, which is how I noticed it.
@GeorgeJahad let us discuss the refactoring changes in the weekly call tomorrow.
