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

feat: added a new text embedded format #836

Closed

Conversation

antonio-pedro99
Copy link
Member

This PR adds a new text embedded format.
This would fix #739

@ppatierno
Copy link
Member

ppatierno commented Sep 20, 2023

@antonio-pedro99 why are you suggesting changes reviewing your own PR instead of just making the changes in the PR itself? All these suggestions spam our inbox :-P
I also see they are about formatting on Javadoc. I would take them out, focusing on the gist of the issue imho.

@antonio-pedro99
Copy link
Member Author

@antonio-pedro99 why are you suggesting changes reviewing your own PR instead of just making the changes in the PR itself? All these suggestions spam our inbox :-P I also see they are about formatting on Javadoc. I would take them out, focusing on the gist of the issue imho.

my bad, sorry for that.

IntelliJ IDEA doing its job :)
sure I will take it out.

@antonio-pedro99
Copy link
Member Author

@ppatierno I think this is the first step!

@scholzj scholzj added this to the 0.27.0 milestone Sep 20, 2023
@ppatierno
Copy link
Member

@antonio-pedro99 are you still working on this? I see it's draft and actually the converter does nothing :-)

@scholzj I am not sure we want to target this for 0.27.0.

@antonio-pedro99
Copy link
Member Author

@antonio-pedro99 are you still working on this? I see it's draft and actually the converter does nothing :-)

@scholzj I am not sure we want to target this for 0.27.0.

Yes, I am still on it. Will push some changes by the end of this week.
Is the PR in the right direction?

@ppatierno
Copy link
Member

Is the PR in the right direction?

Hard to say, the converted does nothing right now :-)

@antonio-pedro99
Copy link
Member Author

Some buildings are failing exactly because of what I said above. I added some tests.

@ppatierno
Copy link
Member

I am not using any decoder, thus the HTTP consumer output is sometime giving ""some value"" and sometime “some value”.
On the another hand, from the consumer console, the output is what we expect. I am not really sure if we want the backslashes .

I am not sure from where you are getting the \ which is something we don't want of course. I haven't got such a problem on my side. Can you provide a reproducer or at least explaining all the steps you are following? (how to produce, how you are consuming, and so on).

@ppatierno
Copy link
Member

@antonio-pedro99 you also need to update the openapiv2.json file. It's the old Swagger 2.0 format but we still support it.

@ppatierno
Copy link
Member

@antonio-pedro99 I think I have got the cause you are getting \" ... you are sending as text but receiving as json and it's normal. When a message is produced with text, it's expected to be consumed as text not as json otherwise the different converter is encoding differently.

@antonio-pedro99
Copy link
Member Author

@antonio-pedro99 I think I have got the cause you are getting \" ... you are sending as text but receiving as json and it's normal. When a message is produced with text, it's expected to be consumed as text not as json otherwise the different converter is encoding differently.

Exactly what I was thinking over the weekend.

@antonio-pedro99
Copy link
Member Author

Another thing I could observe is that not all the output has unescaped backslashes.
Take a look at this output:

[
    {
        "topic": "bridge-quickstart-topic",
        "key": "my-key",
        "value": "This is some text again",
        "partition": 0,
        "offset": 0
    },
    {
        "topic": "bridge-quickstart-topic",
        "key": null,
        "value": "Hellow rold",
        "partition": 0,
        "offset": 1
    },
    {
        "topic": "bridge-quickstart-topic",
        "key": "\"my-key\"",
        "value": "\"sales-lead-0001\"",
        "partition": 0,
        "offset": 2
    },
    {
        "topic": "bridge-quickstart-topic",
        "key": null,
        "value": "\"sales-lead-0003\"",
        "partition": 0,
        "offset": 3
    },
    {
        "topic": "bridge-quickstart-topic",
        "key": "my-key",
        "value": "This is some text again",
        "partition": 0,
        "offset": 4
    }
]

@ppatierno
Copy link
Member

Again it depends how you send the messages by producing with text format or JSON format. If you have a reproducer that would be helpful otherwise I don't see any reasons for them, it's not happening on my side.

@antonio-pedro99
Copy link
Member Author

antonio-pedro99 commented Dec 26, 2023

Again it depends how you send the messages by producing with text format or JSON format. If you have a reproducer that would be helpful otherwise I don't see any reasons for them, it's not happening on my side.

Fr, it is not happing no longer from my side as well.

The same thing is happing with the ConsumerIT.java. While adding tests for the new embedded format, I was producing message like this

  String sentBody = "Simple message as string";
        basicKafkaClient.sendJsonMessagesPlain(topic, 1, sentBody, 0, true);

And then try to read as Text, and of course I am getting the backslashes.

I am trying to use now sendStringMessagesPlain, but getting some time exceeded exceptions. I don't know why yet, but I am investigating further.

And also, I observed that there is no test for the binary format, and I am supposing it is because you weren't able to produce a binary message. Found this instead.

//        kafkaCluster.produce(topic, sentBody.getBytes(), 1, 0);
 // TODO: make client to producer binary data..

Signed-off-by: Antonio Pedro <[email protected]>
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.

Add a new string embedded format
4 participants