dlmarion commented on code in PR #111: URL: https://github.com/apache/accumulo-access/pull/111#discussion_r2924285049
########## modules/core/src/main/java/org/apache/accumulo/access/Authorizations.java: ########## @@ -18,7 +18,6 @@ */ package org.apache.accumulo.access; -import java.io.Serializable; Review Comment: > Was there a reason we didn't want this serializable anymore? Making this class Serializable meant that we had to use a mutable Set implementation in the class (HashSet) and copy the provided set in the constructor and also copy the set to return it in the call to `asSet`. @keith-turner suggested just removing Serializable here and let the user do the serialization themselves if the needed it as it's just a set of strings. > I see AccessExpression is still Serializable. Should we remove that too? We should probably evaluate the classes to see if code complexity and object copying is reduced by implementing [Externalizable](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/io/Externalizable.html) instead of Serializable for Authorizations and AccessExpression. This should allow us to use `Set.copy()` and other non-Serializable immutable types in our code and we just manage the serialization logic ourselves (which may rarely be invoked). -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
