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

EventHubs EventData.getProperties() documentation #31095

Closed
2 tasks done
richorama opened this issue Sep 22, 2022 · 8 comments · Fixed by #31175 or #32517
Closed
2 tasks done

EventHubs EventData.getProperties() documentation #31095

richorama opened this issue Sep 22, 2022 · 8 comments · Fixed by #31175 or #32517
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Event Hubs question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@richorama
Copy link

richorama commented Sep 22, 2022

Is your feature request related to a problem? Please describe.

In the EventHubs client, it is unclear which data types are supported for the properties of EventData class.

https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/eventhubs/azure-messaging-eventhubs/src/main/java/com/azure/messaging/eventhubs/EventData.java#L173-L196

Describe the solution you'd like

Could we have an annotation/documentation that describes what data types are supported, as is the case for the .NET client.

https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/eventhub/Azure.Messaging.EventHubs/src/EventData.cs#L214-L260

Describe alternatives you've considered

It would be even nicer to have this included in the type system, rather than using a Map<String, Object>.

Additional context

n/a

Information Checklist
Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

  • Description Added
  • Expected solution specified
@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Sep 22, 2022
@richorama richorama changed the title [FEATURE REQ] EventHubs EventData.getProperties() documentation Sep 22, 2022
@joshfree joshfree added Event Hubs Client This issue points to a problem in the data-plane of the library. labels Sep 26, 2022
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 26, 2022
@joshfree
Copy link
Member

@conniey @Azure/azsdk-sb-java could you please follow up with @richorama

@conniey
Copy link
Member

conniey commented Sep 27, 2022

Ahh, yeah. Ity should be easy enough to add documentation. thanks!

@conniey
Copy link
Member

conniey commented Nov 2, 2022

The set documented is one supported across other languages. For example, .NET library does not support serializing a map within a map even though proton-j does.

@bvahdat
Copy link

bvahdat commented Nov 10, 2022

The set documented is one supported across other languages. For example, .NET library does not support serializing a map within a map even though proton-j does.

Hi @conniey

The additional types I have asked for here to be included into the documentation, are indeed supported by this repository with the corresponding tests I have added to prove it.

So still not clear to me why these types are not listed.

cc: @richorama

@conniey conniey reopened this Dec 1, 2022
@conniey
Copy link
Member

conniey commented Dec 1, 2022

Hey,

I'm working with our messaging team to add some more clarification around this in our docs.

@conniey
Copy link
Member

conniey commented Dec 7, 2022

To follow up, as of this current release (azure-messaging-eventhubs 5.15.0), the supported types for getProperties() are:

  • String
  • org.apache.qpid.proton.amqp.Symbol
  • Integer
  • Long
  • Short
  • Character
  • Float
  • Double
  • Date

Due to the logic in our EventHubMessageSerializer when we are figuring out if an EventData fits within a batch. Any other types will cause an IllegalArgumentException to be thrown.

For context, this logic was brought over from our legacy Event Hubs client library as-is (AmqpUtil.java). So other types did not work in that library either.

Seeing this gap, I'm working on adding support for more types based on the ones in the AMQP protocol that can be mapped to a class in the Java SDK.

@bvahdat
Copy link

bvahdat commented Dec 8, 2022

To follow up, as of this current release (azure-messaging-eventhubs 5.15.0), the supported types for getProperties() are:

  • String
  • org.apache.qpid.proton.amqp.Symbol
  • Integer
  • Long
  • Short
  • Character
  • Float
  • Double
  • Date

Due to the logic in our EventHubMessageSerializer when we are figuring out if an EventData fits within a batch. Any other types will cause an IllegalArgumentException to be thrown.

For context, this logic was brought over from our legacy Event Hubs client library as-is (AmqpUtil.java). So other types did not work in that library either.

Seeing this gap, I'm working on adding support for more types based on the ones in the AMQP protocol that can be mapped to a class in the Java SDK.

Thanks @conniey for the update.

It means you will also follow up for a fix regarding #31175, right?

@conniey
Copy link
Member

conniey commented Dec 8, 2022

It means you will also follow up for a fix regarding #31175, right?

Yes, this is correct. I'll update the documentation and then create another GitHub issue to address this gap.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. Event Hubs question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
4 participants