Skip to content
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

Open
milleruntime opened this issue Feb 8, 2022 · 12 comments
Open

Refactor FATE #2473

milleruntime opened this issue Feb 8, 2022 · 12 comments
Labels
enhancement This issue describes a new feature, improvement, or optimization.

Comments

@milleruntime
Copy link
Contributor

milleruntime commented Feb 8, 2022

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 like FateEnvironment to pass to the operations we wouldn't have to pass the Manager around.

@milleruntime milleruntime added the enhancement This issue describes a new feature, improvement, or optimization. label Feb 8, 2022
@EdColeman
Copy link
Contributor

Maybe this should consider #1249

@milleruntime
Copy link
Contributor Author

If this was for 3.0, we could probably drop the old Bulk Import. That would help clean up a lot of code.

@milleruntime
Copy link
Contributor Author

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 ServerContext is in server and Fate is in core.

@ctubbsii
Copy link
Member

ctubbsii commented Feb 9, 2022

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.

@keith-turner
Copy link
Contributor

It should be safer to serialize records than general Java objects

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
https://docs.oracle.com/javase/9/docs/api/java/io/ObjectInputFilter.html

@milleruntime
Copy link
Contributor Author

Another improvement that could be included in this change:
Convert FateTxId to a stronger type to prevent coding errors like forgetting to call the method to display the value in HEX.
The code would probably be 10x better if we had a FateTxId type that we used instead of long. Its toString method could consistently format for logging maybe. That could possible encapsulate or extend UUID, but I think we would benefit form a more specific type than UUID or long.

@milleruntime
Copy link
Contributor Author

For serialization of FATE repos, we could use JSON. There is already this in the FateCommand:

private static class ByteArraySerializer implements JsonSerializer<byte[]> {
@Override
public JsonElement serialize(byte[] link, Type type, JsonSerializationContext context) {
return context.serialize(new ByteArrayContainer(link));
}
}

@milleruntime
Copy link
Contributor Author

@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?

@ctubbsii
Copy link
Member

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

@milleruntime
Copy link
Contributor Author

@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

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.

milleruntime added a commit to milleruntime/accumulo that referenced this issue Mar 2, 2022
* 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
milleruntime added a commit to milleruntime/accumulo that referenced this issue Mar 2, 2022
* 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
milleruntime added a commit to milleruntime/accumulo that referenced this issue Mar 8, 2022
* 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
milleruntime added a commit to milleruntime/accumulo that referenced this issue Sep 22, 2022
* 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
@dlmarion
Copy link
Contributor

dlmarion commented Oct 3, 2024

@keith-turner @cshannon - this seems to be about Fate serialization with generics. Is this still an issue with the recent Fate changes?

@cshannon
Copy link
Contributor

cshannon commented Oct 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue describes a new feature, improvement, or optimization.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants