cshannon opened a new pull request, #4437: URL: https://github.com/apache/accumulo/pull/4437
This commit refactors all of the fields to be final for TabletMetadata. The object is already treated as immutable in practice, but previously didn't declare fields final so this could lead to potential future bugs and also means not being able to guarantee the state of a field which is now possible. This was originally done this way to make the construction of the object simpler when constructing from a row as there are so many fields. This refactoring adds a new builder that is used to construct the object and passes that to the constructor so we don't need constructor arguments for all values which simplifies things. This change will now allow verifying state going forward during construction (such as requiring the files map to be non null) which should reduce the chance for future bugs. This also makes the object thread safe by making all fields final and immutable. I came up with the idea to make this change due to the comment [here](https://github.com/apache/accumulo/pull/4404#discussion_r1546945822) . With the field being final we can now guarantee that getLogs() will never return null. A couple notes: 1. I went ahead and added a few non null checks but more could be added, several of the fields can be null so left them alone. 2. I used a supplier for returning the extent because that requires prevEndRow to be fetched which is not always the case. This also led to uncovering a potential bug with getTabletState() when creating `TabletLocationState` where the extent was [passed](https://github.com/apache/accumulo/blob/b912506d43fbac9477aa3efb9ea81d39ce4d1a5c/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java#L374) but could have been null so I fixed that as well and added a null check and also added a check to ensure that prevEndRow as fetched. 3. I updated the `testAllColumns()` test inside of `TabletMetadataTest` to include missing fields and ran with code coverage which verified all the new builder methods were used. This in combination with the existing tests should have enough test coverage. 4. I decided to make this change in main and not 2.1 as it has a small chance to introduce bugs and the metadata schema should not be changed there anyways. When this is merged to main I can fix the merge issues and update elasticity as there are some extra fields there. -- 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: notifications-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
