You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/08/03 23:10:56 UTC

[GitHub] [ozone] GeorgeJahad opened a new pull request, #3653: HDDS-6964. [Snapshot] Split out shared "Path based access" code from OM.

GeorgeJahad opened a new pull request, #3653:
URL: https://github.com/apache/ozone/pull/3653

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

### Locks and Metrics
The code in OmMReader makes extensive use of the OzoneManagerLock and OMMetrics classes. Snapshotting will need its own version of those so I've created two new interfaces that correspond to them: OmLock and OmMReaderMetrics. These interfaces are 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.)

Note that snapshot version of the lock won't be a lock at all. It just seemed cleaner to use an interface instead of putting "if statements" everywhere. Instead of a real lock, the snapshot version will always return true on "acquireReadLock()" and false on "acquireWriteLock()".

### 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](https://github.com/apache/ozone/blob/18ea21661/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/IOzoneAcl.java#L40-L60)

### 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](https://github.com/apache/ozone/blob/dd01353137322009edba4e20f07f697ff22d2a82/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L694).

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](https://github.com/apache/ozone/blob/18ea21661/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L568).

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](https://github.com/GeorgeJahad/ozone/compare/b2ffbbed2e8d9ea815cff4d2306e2798c3dad84a..compareMReader1?w=1)

As you'll see, these are the main things changed, in the code moved to the OmMReader:

1. A few private methods were made package-private.
2. 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.)
3. Changed the omRpcAddress access to "ozoneManager.getOmRpcServerAddr()" as mentioned above.

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

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

To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org