-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
possible move to Jackson from Gson for kafka #655
Comments
Hi @a1shadows , |
Hi @a1shadows E.g. in kafka-testing/src/test/resources/kafka/produce/test_kafka_produce_int_key.json, the request contains an "Integer" key "101" in the produce operation, but during the unload operation the assertion checks for a "Double" key. {
"scenarioName": "Produce a message to kafka topic - int Key",
"steps": [
{...
"operation": "produce",
"request": {
"recordType" : "RAW",
"records": [
{
**"key": 101,**
"value": "Hello World"
}
]
}, ...
},
{ ...
"operation": "unload",
...
"assertions": {
"size": 1,
"records": [
{
**"key": 101.0,**
"value": "Hello World"
}
]
}
}
]
} Moving to Jackson, I did not find a suitable build-in replacement for this mechanism. So we would have recreate this behaviour programmatically to prevent the tests from failing. I don't know whether this implicit conversion Integer->Double just sneaked in because GSON had been used or if it is an important feature. Could we change "key" in the produce section of the unit tests to double? This would break the current functionality, but it would be more consistent in my opinion. |
Hey, I'll take a look at this and get back to you |
Hey Guys @baulea, @a1shadows, Thanks for looking at it. But imo gson is still fine as long as it doesn't do any harm, also it's great where it has solved a usecase easily. So my advice would be to keep this as a low proprity TECHDEBT or withdraw this ticket until it blocks anyone or any problem scenario. Thanks for understanding... |
based on discussion with @baulea and @authorjapps , we need to investigate why Gson was used and if we should move fully to Jackson.
discussed in #648
The text was updated successfully, but these errors were encountered: