feat: add dual source mode for addRelationship processing #466
Conversation
| @@ -0,0 +1,9 @@ | |||
| namespace com.linkedin.testing.localrelationship | |||
|
|
|||
| @pairings = [ { | |||
There was a problem hiding this comment.
I think we're not using pairings annotation in v2 anymore
There was a problem hiding this comment.
right, let me change this to V2 completely.
| "source": "com.linkedin.testing.urn.BarUrn" | ||
| } ] | ||
| @gma.model = "RELATIONSHIP" | ||
| record BelongsToV2 includes BaseRelationship { |
There was a problem hiding this comment.
I don't think we use BaseRelationship any more either
| @@ -104,7 +105,8 @@ public enum SchemaConfig { | |||
| */ | |||
| public enum RelationshipSource { | |||
| ASPECT_METADATA, // Look at the aspect model and extract any relationship from its fields | |||
| RELATIONSHIP_BUILDERS // Use the relationship registry, which relies on relationship builders | |||
| RELATIONSHIP_BUILDERS, // Use the relationship registry, which relies on relationship builders | |||
| DUAL_SOURCE | |||
There was a problem hiding this comment.
Yes, that's what I had in mind. We can try it for 1 relationship type first, maybe OwnedBy since it is relatively small in number and also since no one is using it now.
ybz1013
force-pushed
the
yanbzhao/dual-write
branch
from
40445f2 to
ae808f3
Compare
| @@ -104,7 +105,8 @@ public enum SchemaConfig { | |||
| */ | |||
| public enum RelationshipSource { | |||
| ASPECT_METADATA, // Look at the aspect model and extract any relationship from its fields | |||
| RELATIONSHIP_BUILDERS // Use the relationship registry, which relies on relationship builders | |||
| RELATIONSHIP_BUILDERS, // Use the relationship registry, which relies on relationship builders | |||
| DUAL_SOURCE | |||
There was a problem hiding this comment.
Yes, that's what I had in mind. We can try it for 1 relationship type first, maybe OwnedBy since it is relatively small in number and also since no one is using it now.
| if (_relationshipSource == RelationshipSource.ASPECT_METADATA) { | ||
| needsToBuildFromAspect = true; |
There was a problem hiding this comment.
I think it's easier to just:
boolean needsToBuildFromAspect = _relationshipSource == RelationshipSource.ASPECT_METADATA || _relationshipSource == RelationshipSource.DUAL_SOURCE;
boolean needsToBuildFromRelationshipBuilders = _relationshipSource == RelationshipSource.RELATIONSHIP_BUILDERS || _relationshipSource == RelationshipSource.DUAL_SOURCE;
|
|
||
| // if DUAL_SOURCE mode, verify that the relationships extracted from aspect and built from relationship builders have the same source/destination pairs, | ||
| // with different relationship types. Else, throw an exception. | ||
| if (areConsistentLocalRelationshipUpdates(updatesExtractedFromAspect, updatesBuiltFromRelationshipBuilders)) { |
There was a problem hiding this comment.
don't you need to check that it is DUAL_SOURCE mode? otherwise, this if check will always throw an exception?
There was a problem hiding this comment.
I was tired and writing bugs yesterday lol. The code in PR is really bad.
| */ | ||
| private boolean areConsistentLocalRelationshipUpdates(List<LocalRelationshipUpdates> updatesExtractedFromAspect, | ||
| List<LocalRelationshipUpdates> updatesBuiltFromRelationshipBuilders) { | ||
| if (updatesExtractedFromAspect.isEmpty() || updatesBuiltFromRelationshipBuilders.isEmpty()) { |
There was a problem hiding this comment.
It is possible that this is true while there are still relationships extracted from the relationship builders.
AspectFoo {
bars = [b]
belongsTos = []
}
The relationship builder can return {A->B} but belongsTos is empty. This should throw an error, not return true.
| } | ||
|
|
||
| // must have different relationship types | ||
| Set relationshipTypes = updatesExtractedFromAspect.stream() |
There was a problem hiding this comment.
nit: rename to aspectExtractedRelationshipTypes to make it clearer
|
|
||
| // TODO: update aspect in unit tests to v2, as aspect should aways be v2, if aspect is v1, we should throw exception | ||
| // must have the same source/destination pairs | ||
| Set<String> sourceDestinationPairs = (Set<String>) updatesExtractedFromAspect.stream() |
There was a problem hiding this comment.
QQ: how does this actually compare source1 = source2 and dest1 = dest2? I might be missing something
There was a problem hiding this comment.
the relationship will be converted to string "{source:"something","destination":"something"}". I think the fields will always come in this order.
| .map(relationship -> relationship.toString()) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| if (!sourceDestinationPairs.equals(s)) { |
There was a problem hiding this comment.
nit: return sourceDestinationPairs.equals(s)
Summary
This PR will enable the EBeanLocalDAO to process extract relationships from both aspect and local relationship builder. The assumption is the aspect will have V2 relationships and local relationship builder will have V2 relationships, but the source/destination pairing value should be the same. Then the relationships will be written to both V1 and V2 tables.
I.e., during V1 -> V2 transition, the relationships will be extracted from both sources and be written to v1 and v2 tables separately.
also refactored old unit tests to have aspect use v2 relationships.
Testing Done
./gradlew build
unit tests
Checklist