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

#313 solved serializers casting problems using SpecificDatumWriter #315

Merged
merged 14 commits into from
Dec 19, 2022

Conversation

davidgarciago
Copy link
Contributor

The casting problem of long_timestamp-millis gets solved when using the GenericDatumWriter. Despite the lack of documentation it seems that we should use SpecificDatumWriter only when needed.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for collaborating with the project to help us to improve!!'

pedror19
pedror19 previously approved these changes Dec 1, 2022
Copy link
Contributor

@dianaGomezGar dianaGomezGar left a comment

Choose a reason for hiding this comment

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

I think the pom version needs to be updated.

Copy link
Contributor

@jemacineiras jemacineiras left a comment

Choose a reason for hiding this comment

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

Pom version should be updated

…b.com:corunet/kloadgen into 313-issue-long-timestamp-millis-trouble-to-cast
@davidgarciago
Copy link
Contributor Author

I have added a new docker file from the #303 issue branch. I think it can be usefull to test the plugin.

dianaGomezGar
dianaGomezGar previously approved these changes Dec 5, 2022
@davidgarciago
Copy link
Contributor Author

davidgarciago commented Dec 12, 2022

All changes requested have been committed. Please @jemacineiras review them when you have time.

@davidgarciago
Copy link
Contributor Author

I have added new tests for the serializers. I also removed the use of SpecificDatumWritter from both serializers affected by the issue because it was never used. The reason of this was that serializers data was always of type GenericRecord and not SpecificRecord.

@@ -21,7 +24,12 @@

@Override
public final byte[] serialize(final String s, final T data) {
final var writer = new SpecificDatumWriter<>(data.getSchema());
DatumWriter<T> writer;
if (data instanceof SpecificRecord) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing test

@@ -23,7 +26,13 @@
@Override
public final byte[] serialize(final String topic, final T data) {

final var writer = new SpecificDatumWriter<>(data.getSchema());
DatumWriter<T> writer;
if (data instanceof SpecificRecord) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing test

@davidgarciago davidgarciago merged commit cd006c0 into master Dec 19, 2022
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.

5 participants