Conversation
| public void onTableRemoved(TableMetadata table) { | ||
| synchronized (mappers) { | ||
| Iterator<Mapper<?>> it = mappers.values().iterator(); | ||
| while (it.hasNext()) { |
There was a problem hiding this comment.
Why not a regular for-each loop?
There was a problem hiding this comment.
Nevermind, I hadn't noticed the remove() call.
|
Squashed and rebased on top of current 3.0.x. |
| } | ||
|
|
||
| @Override | ||
| public void onTableChanged(TableMetadata current, TableMetadata previous) { |
There was a problem hiding this comment.
On the case of a user type being changed, it looks like it's removed from mappers, but then readded via getUDTCodec, but this doesn't do the same for table via getMapper, should it?
There was a problem hiding this comment.
Ah maybe it is needed in the UDT case because it updates the Codec, where no analog is needed for table. I suppose only benefit is maybe we'd be able to log the error here instead of when the user tries to use the mapper.
There was a problem hiding this comment.
I suppose the other issue is replacing the Mapper doesn't do a lot of good since the user would still hold a reference to the old one so it makes sense to not attempt to replace it.
|
rebased (3.0.x had some test fixes) and added some tests and documentation. I also made a slight change so lgtm 👍 assuming CI looks good. |
| mapper.addColumns( | ||
| createColumnMappers(pks, factory, mapper.entityClass, mappingManager, columnCounter, tableMetadata, ksName, tableName), | ||
| createColumnMappers(ccs, factory, mapper.entityClass, mappingManager, columnCounter, tableMetadata, ksName, tableName), | ||
| createColumnMappers(rgs, factory, mapper.entityClass, mappingManager, columnCounter, tableMetadata, ksName, tableName)); |
There was a problem hiding this comment.
ksName and tableName could be retrieved from tableMetadata instead of passing them as parameters (also applies to createFieldMappers below).
There was a problem hiding this comment.
That's true but frankly I prefer to leave it as is for practical reasons:
- Quoting of case-sensitive identifiers is not very consistent, and is done in different places, so should we quote or not the result of
TableMetadata.getName()? The code coverage for case-sensitive use cases is not very good, so let's not introduce regressions for that. - In JAVA-541 + JAVA-636 + JAVA-984: Polymorphism support to object mapper #686 I added specific tests for case-sensitiveness and also applied a methodic quoting of identifiers). See btw JAVA-564.
- The less we modify this class the easier it will be to merge it with JAVA-541 :)
Great tests, thanks for that! |
The mapping manager subscribes to schema change events and reacts to table and UDT removed / modified notifications: a warning is logged and, for modifications, the manager tries to replace the mapper or codec with a new one (which may or may not work depending on the nature of the change and whether client code kept a reference to the old mapper instance). In addition, table/UDT names and column/field names are validated earlier, in a fail-fast attempt to detect wrong configurations.
This PR proposes the following enhancements:
MappedUDTCodecwith a new one;Mapper) in a fail-fast attempt to detect wrong configurations;This however cannot prevent existing
Mapperinstances from misbehaving, i.e. if a user creates aMapperinstance, then issues anALTERstatement, thatMapperinstance would be stale and most likely, unusable.