Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #567: ( Impact on BC/DR - Data loss ) - High priority, Nodes with PointValue property cannot be successfully sink to Neo4j With Neo4j Streams Plugin #583

Merged
merged 1 commit into from
Oct 8, 2023

Conversation

conker84
Copy link
Contributor

@conker84 conker84 commented Aug 22, 2023

Fixes #567

One sentence summary of the change.

Proposed Changes (Mandatory)

A brief list of proposed changes in order to fix the issue:

  • fixed the bug
  • added test
  • removed kafka-connect from the build as we rely on branch 5.0 for that

@conker84 conker84 marked this pull request as draft August 22, 2023 15:18
@conker84 conker84 requested a review from ali-ince August 22, 2023 20:18
@conker84 conker84 marked this pull request as ready for review August 22, 2023 20:18
Copy link
Contributor

@ali-ince ali-ince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about AVRO support? Is it supported out of the box, or do we need to implement that too?

@@ -33,6 +54,42 @@ fun Point.toStreamsPoint(): StreamsPoint {
}
}

fun Map<String, Any>.toMapValue(): MapValue {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we maybe use org.neo4j.values.virtual.MapValueBuilder instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

override fun deserialize(parser: JsonParser, context: DeserializationContext): EVENT {
val root: JsonNode = parser.codec.readTree(parser)
val meta = JSONUtils.convertValue<Meta>(root["meta"])
val schema = JSONUtils.convertValue<Schema>(root["schema"])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

am I correct to think that if the user did not include schema information, this still won't work? If so, what would the user do to create point values from messages generated by non-neo4j applications? Regardless, I think we need to extend the documentation accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not advocate to the users to use this message created by themselves otherwise we need to document all the possible permutations of schema and so on. So for me, this is about the CDC message which contains the info. If it doesn't it does mean that the message is not well-formed.
Maybe is worth adding docs or at least a link to the CDC doc page (https://neo4j.com/docs/kafka-streams/producer/#_transaction_event_handler) in the Kafka Connect PR where there is no direct reference to the message structure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, looks like we are also missing the type for geo properties on the sample messages on the page you linked. Would you please update that page and also a separate page (probably best to avoid links) that includes similar sample messages for the kafka connect documentation?

pom.xml Outdated
@@ -43,7 +43,7 @@
<modules>
<module>common</module>
<module>test-support</module>
<module>kafka-connect-neo4j</module>
<!-- <module>kafka-connect-neo4j</module>-->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we remove this and even the source folder, maybe as a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I commented this to open a discussion about it. As we don't need it anymore we can get rid of it in this branch

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed.

@conker84
Copy link
Contributor Author

How about AVRO support? Is it supported out of the box, or do we need to implement that too?

The Streams Transaction Event Handler does not produce messages as AVRO format.

@conker84 conker84 force-pushed the issue_567 branch 4 times, most recently from bc12284 to 4f092fb Compare September 3, 2023 12:29
@conker84 conker84 force-pushed the issue_567 branch 17 times, most recently from 7341d9d to f8e65d7 Compare September 21, 2023 10:10
…ity, Nodes with PointValue property cannot be successfully sink to Neo4j With Neo4j Streams Plugin
@conker84 conker84 merged commit 9eb8140 into neo4j-contrib:4.1 Oct 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants