Add TarantoolTupleResultMapper factory #322
Conversation
src/main/java/io/tarantool/driver/mappers/TarantoolTupleResultMapperFactory.java
Show resolved
Hide resolved
src/main/java/io/tarantool/driver/mappers/TarantoolTupleResultMapperFactoryImpl.java
Outdated
Show resolved
Hide resolved
src/test/java/io/tarantool/driver/benchmark/TarantoolSetup.java
Outdated
Show resolved
Hide resolved
ArtDu
force-pushed
the
refactor/new/factories
branch
4 times, most recently
from
69533da to
4ce4c42
Compare
There was a problem hiding this comment.
One important test kind that is missing - we need to test that several consequent and concurrent invocations of TarantoolTupleResultMapperFactory methods do not cause any problems with wrong mapper stacks. That means (until we haven't implemented any smart caching) a new isolated mapping stack should be created for each request. Otherwise, we immediately will have serious problems that are hard to debug
| @@ -111,6 +115,7 @@ public AbstractTarantoolClient(TarantoolClientConfig config, TarantoolConnection | |||
|
|
|||
| this.config = config; | |||
| this.mapperFactoryFactory = new ResultMapperFactoryFactoryImpl(); | |||
| this.resultMapperFactory = TarantoolTupleResultMapperFactoryImpl.getInstance(); | |||
There was a problem hiding this comment.
It does not look right to put a concrete tuple factory implementation in the abstract client, given that the Tarantool tuple class is a generic parameter. I believe it should be instantiated in the corresponding final class or some intermediate one. In the root abstract class, we may put an abstract method for it with a generic type.
There was a problem hiding this comment.
Moved factory to ResultMapperFactoryFactory
| @@ -491,4 +492,6 @@ <T, R extends List<T>> CompletableFuture<R> callForMultiResult( | |||
| * @return result mapper factory instances factory instance | |||
| */ | |||
| ResultMapperFactoryFactory getResultMapperFactoryFactory(); | |||
|
|
|||
| TarantoolTupleResultMapperFactory getTarantoolTupleResultMapperFactory(); | |||
There was a problem hiding this comment.
Looking at this again, I strongly believe that we need to have this factory method in the ResultMapperFactoryFactory. Otherwise it looks like an orphan
There was a problem hiding this comment.
Looking at this again, I strongly believe that we need to have this factory method in the ResultMapperFactoryFactory. Otherwise it looks like an orphan
I don't want to see this, it's awful
client.getResultMapperFactoryFactory().getTarantoolTupleResultMapperFactory()
.withSingleValueRowsMetadataToTarantoolTupleResultMapperThere was a problem hiding this comment.
C'est la vie \o/
It's not used everyday, so an orphan method in API will confuse the users more. It's a classical leaking abstraction.
There was a problem hiding this comment.
It's not used everyday, so an orphan method in API will confuse the users more
It's just a shortcut for main mappers. But I'll move if you don't change your mind about using it here where is it now. I need it to futher autoTupleConverter
In my opinion, it's hard to use from client.getResultMapperFactoryFactory().* cause there are so many methods. It's so difficult for people that I suggeseted to use these methods. Every time I started to think how does these ugly structures. And the sum thing that these structure don't give a perfomance in comparing to http because we have a lot of problems with those.
@Elishtar What do you think about it? I think we need third opinion
There was a problem hiding this comment.
You are leaking a concrete TarantoolTuple implementation to the TarantoolClient interface which has this type as a generic type. We should not do this by any chance. It harms more than the "economy" of symbols in one place or the "ugliness" of this factory name, That's the complexity of the source object that we are modeling and we need to get it away from the users by providing non-confusing and clear structures to them.
There was a problem hiding this comment.
It's better to remove the method getTarantoolTupleResultMapperFactory() itself and use this factory as a singleton than to add a new method that is necessary only for internal purposes. So I'd rather vote for removing getTarantoolTupleResultMapperFactory() from the TarantoolClient interface instead
There was a problem hiding this comment.
@akudiyar for me ResultMapperFactoryFactory looks very strange and over complexity, maybe better use few different factories?
There was a problem hiding this comment.
@Elishtar the initial goal of this factory was to provide helper factory methods for the common cases like this one (a concrete tuple implementation + a concrete result type). Eventually these factory methods were intended to return singleton references. Changing this to individual factory/singleton variable instantiations may work, but to me looks like less readable and harder for tracking and altering the places of factories usages.
There was a problem hiding this comment.
We discussed that we move this factory to ResultMapperFactoryFactory now and that we should discuss API in more detailed later
ArtDu
force-pushed
the
refactor/new/factories
branch
from
4ce4c42 to
405e91e
Compare
We need it to create universal mapper that can parse crud and box responses.
Also with this factory we can't see inherited methods.
I haven't forgotten about:
Related issues:
Closes #321