Refactor LR Publication responses to use partial metadata #17960
Conversation
|
re-test this please |
server/src/test/java/io/crate/integrationtests/MetadataTrackerITest.java
Outdated
Show resolved
Hide resolved
server/src/main/java/io/crate/replication/logical/action/PublicationsStateAction.java
Show resolved
Hide resolved
server/src/main/java/io/crate/replication/logical/metadata/Publication.java
Outdated
Show resolved
Hide resolved
| public static AllocPosition forTable(RelationMetadata.Table table) { | ||
| int maxPosition = 0; | ||
| HashMap<ColumnIdent, Integer> colPositions = new HashMap<>(); | ||
| for (Reference reference : table.columns()) { |
There was a problem hiding this comment.
If I remember right table.columns() also contains deleted columns - might need to add a isDropped() check here to account for that.
There was a problem hiding this comment.
hm, but also positions of dropped columns should be resolvable. E.g. we added concrete logic here https://github.com/crate/crate/blob/master/server/src/main/java/io/crate/execution/ddl/tables/MappingUtil.java#L65 to also resolve positions of dropped columns.
There was a problem hiding this comment.
So I'd keep it like that or any objections @mfussenegger ?
There was a problem hiding this comment.
My main concern was that the put of a dropped column could override it's replacement, which would mean the lookup would get the old position. Not sure if that causes issues here but could use a putIfAbsent for dropped cols.
If it's not an issue, fine to keep as is.
There was a problem hiding this comment.
I see, I think you're right and this can be an issue. I've adapted the code to ensure dropped cols won't override existing entries by using a dedicated map.
seut
force-pushed
the
s/lr-publication-metadata
branch
from
52dd24f to
a35d8fe
Compare
seut
force-pushed
the
s/lr-publication-metadata
branch
from
a35d8fe to
59c0149
Compare
seut
force-pushed
the
s/lr-publication-metadata
branch
from
59c0149 to
0254f05
Compare
|
This pull request has been removed from the queue for the following reason: Pull request #17960 has been dequeued, merge conditions unmatch:
You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
Instead of responding concrete special entities for describing new or changed tables, the logical replication PublicationState action returns a partial Metadata containing all relevant information. This removes the handling of these special entities and also simplifies any future changes inside the table structures. The new partial Metadata will contain the new RelationMetadata entities, any deprecated template handling is removed. Requests from/to older nodes are handled transparently Relates to #17518, #17769.
seut
force-pushed
the
s/lr-publication-metadata
branch
from
b98a01c to
208d742
Compare
|
This pull request has been removed from the queue for the following reason: Pull request #17960 has been dequeued, merge conditions unmatch:
You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
seut
added
the
ready-to-merge
|
@Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
Instead of responding concrete special entities for describing new or changed tables, the logical replication PublicationState action returns a partial Metadata containing all relevant information. This removes the handling of these special entities and also simplifies any future changes inside the table structures.
The new partial Metadata will contain the new RelationMetadata entities, any deprecated template handling is removed. Requests from/to older nodes are handled transparently
Relates to #17518, #17769.