Copier context #196
Copier context #196
Conversation
temporarily to get test code compiling quicker) * WIP on tests passing
| } | ||
|
|
||
| /** | ||
| * @deprecated As of release 16.3.0, replaced by {@link #newInstance(CopierContext)}. |
There was a problem hiding this comment.
Instead of deprecating these we could remove them and then just have the method above and clearly document in the CHANGELOG what the required change is for this. It's basically just updating any custom CopierFactory implementations to implement the new method and pull the values they need out of the CopierContext instead of having them passed as method arguments. Thoughts?
There was a problem hiding this comment.
I'd still rather deprecate it and, in case, have it removed later on.
There was a problem hiding this comment.
deprecate and remove is the more friendly way, I'm ok either way though it's not a big change to do.
There was a problem hiding this comment.
OK, let's go with a 16.3.0 release that is backwards compatible, I'll update the changelog accordingly.
| String replicaDatabaseName, | ||
| String replicaTableName, | ||
| String replicaTableLocation) { | ||
| Replica replica) { |
|
|
||
| import com.hotels.bdp.circustrain.api.conf.TableReplication; | ||
|
|
||
| public class CopierContext { |
There was a problem hiding this comment.
thoughts on making this immutable? Is there a risk of a copier accidentally change one of these fields which might affect further copiers in the chain?
There was a problem hiding this comment.
Yeah, I thought about making this immutable and having a builder instead of this big constructor but this was quicker and easier to do in terms of updating the existing code. However the TableReplication isn't immutable so we'd only be halfway there unless I looked into that too (not sure that's really possible as it maps directly to the config which needs to set values).
If we didn't want to use a builder I could add 2 more constructors which allow the TableReplication to be passed in too and then I make immutable copies of the List and Map member variables and we live with TableReplication not being immutable.
I can do either (or if someone has better suggestions) if we think it's worth it?
There was a problem hiding this comment.
I think since this is a new one we make the good example and make it immutable. I'm ok leaving TableReplication for another time as that will be backward incompatible change I guess.
There was a problem hiding this comment.
Isn't there any place where the context may actually need to be updated?
There was a problem hiding this comment.
yeah I was also thinking about that, not sure what we want to achieve. The order in which copiers are run is guaranteed by spring so you could have a copier relying on another copier updating the context. If that's they way we want to go then obviously we don't want to make it immutable. Could argue either way I guess :)
There was a problem hiding this comment.
the class needs to be final
There was a problem hiding this comment.
It does! Good spot!
There was a problem hiding this comment.
and because TableReplication isn't immutable you need to copy it as well. Path also isn't :/ it's a pandemic. Not sure how much you want to open that can of worms. I can live with a not fully immutable class for the sake of pragmatism.
There was a problem hiding this comment.
Yeah, you're right, I think we either leave as it is now and live with a decent attempt but TableReplication and Path are mutable or we just roll the whole set of immutable changes back.
Fixes #195
This PR introduces a
CopierContextclass to hold the values that were being passed to theCopierFactorybefore, and then adds aTableReplicationto this which in turns holds the additional values that were requested in the issue (source database and table names). This allows us to add more values to CopierContext in the past without breaking backwards compatibility which is the situation we're currently in due to the interface methods directly containing the values as arguments.The changes in this PR are currently backwards-compatible by using a default method in the
CopierFactoryinterface but I'll add some comments on this as another option would be to make a backwards incompatible change as it's quite easy to resolve downstream.