-
Notifications
You must be signed in to change notification settings - Fork 454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor FATE #2473
Comments
Maybe this should consider #1249 |
If this was for 3.0, we could probably drop the old Bulk Import. That would help clean up a lot of code. |
One idea I had was to create an Environment interface to pass between FATE and the Manager. The tricky part is that some of the FATE code is in core (the interface, framework and some implementation) while most of it is in the same package as the Manger. I thought we could do this: public interface RepoEnv { public ServerContext getContext(); } But that won't work because |
On serialization, Java 17 has the new record keyword. It should be safer to serialize records than general Java objects, as we've done in the past. I have not yet used records, and we aren't using Java 17 yet... but could for 3.0 development. |
Looking around for what we could do now to make things safer, found the following. Maybe we could use the new ObjectInputFilter added in Java 9 to explicitly limit what classes FATE can deserialize. https://www.oracle.com/java/technologies/javase/seccodeguide.html#8 |
Another improvement that could be included in this change: |
For serialization of FATE repos, we could use JSON. There is already this in the FateCommand: accumulo/shell/src/main/java/org/apache/accumulo/shell/commands/fate/FateCommand.java Lines 85 to 90 in 0fbf9cd
|
@ctubbsii @keith-turner I have been experimenting with JSON serialization in FATE. There are already some classes to serialize the data for displaying with the Fate command. What do you guys think of trying to change the serialization to JSON in 2.1? |
I think that would probably be too big and too disruptive for 2.1 as we're trying to wrap up major features for 2.1. But, I'm not opposed to the idea, generally. However, I still think it's worth looking into newer Java serialization for record objects in 3.0. |
After some experimentation, that was my feeling as well. The generics combined with the complexity of the different interfaces really make it difficult to make any small changes. |
* Refactor Fate to allow removing of generics and be able to improve serialization * Move Fate classes from core to Manager * Create new FateZooStore in core and make ZooStore extend it, consolidating methods between the 2 classes. This allows sharing between Manager and Shell * Extract inner classes to allow reuse and consolidation across packages * Drop Generics from many different Fate interfaces and classes * Refactor FateCommand and create a FateCommandTest * Combine Fate admin utilities into one FateAdmin * Drop master FateAdmin * Drop AdminUtil in favor of FateCommandHelper * Rename method from checkGlobalLock() to isManagerLockValid() * Move TStatus to core and rename to FateTransactionStatus * Closes apache#2473
* Refactor Fate to allow removing of generics and be able to improve serialization * Move Fate classes from core to Manager * Create new FateZooStore in core and make ZooStore extend it, consolidating methods between the 2 classes. This allows sharing between Manager and Shell * Extract inner classes to allow reuse and consolidation across packages * Drop Generics from many different Fate interfaces and classes * Refactor FateCommand and create a FateCommandTest * Combine Fate admin utilities into one FateAdmin * Drop AdminUtil in favor of FateCommandHelper * Rename method from checkGlobalLock() to isManagerLockValid() * Move TStatus to core and rename to FateTransactionStatus * Closes apache#2473 * Create new Progress object * Drop master FateAdmin
* Refactor Fate to allow removing of generics and be able to improve serialization * Move Fate classes from core to Manager * Create new FateZooStore in core and make ZooStore extend it, consolidating methods between the 2 classes. This allows sharing between Manager and Shell * Extract inner classes to allow reuse and consolidation across packages * Drop Generics from many different Fate interfaces and classes * Refactor FateCommand and create a FateCommandTest * Combine Fate admin utilities into one FateAdmin * Drop AdminUtil in favor of FateCommandHelper * Rename method from checkGlobalLock() to isManagerLockValid() * Move TStatus to core and rename to FateTransactionStatus * Closes apache#2473 * Create new Progress object * Drop master FateAdmin
* Refactor Fate to allow removing of generics and be able to improve serialization * Move Fate classes from core to Manager * Create new FateZooStore in core and make ZooStore extend it, consolidating methods between the 2 classes. This allows sharing between Manager and Shell * Extract inner classes to allow reuse and consolidation across packages * Drop Generics from many different Fate interfaces and classes * Refactor FateCommand and create a FateCommandTest * Combine Fate admin utilities into one FateAdmin * Drop AdminUtil in favor of FateCommandHelper * Rename method from checkGlobalLock() to isManagerLockValid() * Move TStatus to core and rename to FateTransactionStatus * Closes apache#2473 * Create new Progress object * Drop master FateAdmin
@keith-turner @cshannon - this seems to be about Fate serialization with generics. Is this still an issue with the recent Fate changes? |
The generics haven't changed, but I don't really think it's a big deal and I personally don't see a need to refactor it and not sure it would actually help anything. |
I was looking at the warnings we suppress in
ZooStore
and they all seem to stem from the serialization of the generics. I was wondering if it would be better to refactor out the generic types so we don't have to arbitrarily serialize objects and blindly cast them. This would be a bit of work but it might not be too bad. I think if we create an interface likeFateEnvironment
to pass to the operations we wouldn't have to pass the Manager around.The text was updated successfully, but these errors were encountered: