Mutable objects used for immutable values #772

[fabio-rizzo-01]

Updated Polaris entity objects to use builders instead of normal constructors and removed all the set methods to guarantee object immutability.
All the other changes are refactoring based on those changes.

[snazy]

I like the removal of all the setters!
However, since these types now look pretty much how those would look like when using immutables, mind going down that route? That would also remove all the hand-written Builders.

[pingtimeout]

I agree with Robert. I think this PR does not fully embrace the immutable pattern.

• The class fields are not final, which would clearly establish the intent of those objects being immutable.
• Adding the Builder subclasses adds a lot of code that could be automatically generated
• (nit) It could even be possible to annotate the classes with @Immutable (from jcip) to enable an extra verification step in the IDE

It would be good to explore the immutables library to address some of these points.

Note that this PR contains a removal of some utility copy-constructors (PolarisEntityCore from PolarisBaseEntity), which might not be related?

[singhpk234]

+1 to PolarisImmutables

[fabio-rizzo-01]

I like the removal of all the setters! However, since these types now look pretty much how those would look like when using immutables, mind going down that route? That would also remove all the hand-written Builders.

From the bug description I thought the point was to make those object not mutable, i didn't think there was a library already there hence why I created the builders. can you guys point me in the right direction thx ? @singhpk234

[eric-maynard]

@fabio-rizzo-01 I think the ask is that you use this interface.

Besides this, I think we should try to minimize the changes that callers need to make as much as possible.

[fabio-rizzo-01]

@fabio-rizzo-01 I think the ask is that you use this interface.

Besides this, I think we should try to minimize the changes that callers need to make as much as possible.

Looking at that interface and seeing where it is used, it seems to me that using it would require even more changes that what I already did.

Taking PolarisEntityCore as an example, to convert that to an immutable object I'd have to create an interface (something like PolarisEntityCoreInt) that uses PolarisImmutable annotation, that would generate a class called Immutable PolarisEntityCoreInt. So all the code that is using PolarisEntityCore object it needs to be updated to use the new immutable object.

Is this what you guys are suggesting ? @snazy @singhpk234

[eric-maynard]

Personally I'm fine with you not using @PolarisImmutable -- I still haven't figured out why we need that or why my IDE so often doesn't seem to like it:

But I do think this PR makes quite a few changes, such as those to TransactionalMetaStoreManagerImpl.java, which could be reduced or elimated. For example, does the method setGrantRecordsVersion really need to go away? If it does, can we implement a cloning method like withGrantRecordsVersion?