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 the jsonconfig conflicts #241

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

BartJanvanAssen
Copy link

Added default config options for json serializers so it's not conflicting with the user's default project settings.
issue is over here: #169

/// <summary>
/// JsonConvert Options
/// </summary>
public static class JsonConvertOptions
Copy link
Member

@serkantkaraca serkantkaraca Feb 7, 2018

Choose a reason for hiding this comment

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

JsonConvertOptions [](start = 24, length = 18)

Instead of this, can you create a static instance of JsonSerializer?

JsonSerializer.Create Method
Creates a new JsonSerializer instance. The JsonSerializer will not use default settings from DefaultSettings.

https://www.newtonsoft.com/json/help/html/M_Newtonsoft_Json_JsonSerializer_Create.htm #Closed

Copy link
Author

Choose a reason for hiding this comment

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

Yep, though you're creating a custom JsonSerializerSettings obj to be able to use the JsonConvert.SerializeObject function.

I just checked another Azure library, they wrapped the entire json settings into a class, resulting pretty much the same I think...

We cannot create a non-default variant of the JsonConvert unfortunately. We could go for the webjob's implementation although we have to add the ContractResolver property otherwise I'm still conflicting with the snake case my code uses. (because they use JsonSerializer.CreateDefault it reuses the default AND the custom settings of the lib) Looking forward to your opinion on this :)

@serkantkaraca
Copy link
Member

serkantkaraca commented Feb 23, 2018

Another thing I realized. We should not break existing leases. This change should only help on the new leases and clients with existing leases should continue working w/o a need to set the options. Have you tested this kind of scenario with your change?

@BartJanvanAssen
Copy link
Author

I'm not 100% sure if i follow you... I have this change working now for some days and it's operating fine :) This change involves that the encapsulated settings of the library stay the same whatever the user defined to their default settings. As Azure works fine with CamelCasing i think this is sufficient.
What do you mean with: "with existing leases should continue working w/o a need to set the options"?
When people have their settings based upon any other kind of casing, they probably do similar tricks to get this working i guess... And when this gets merged in the code the EH workers will reboot i guess before they can start using the new lib files so all connections will be new ones.

@serkantkaraca
Copy link
Member

I just want to make sure the change doesn't introduce a breaking change for the existing leases. We can assume it won't break but we cannot know for that real before giving a test.

Here is a basic scenario:

  • Start receiving with current client. Leases will be created with default casing.
  • Stop the receiver client and restart it with new EH library.
  • Existing leases should continue working.

/// Get the JsonConvert formatting options. so it's not conflicting / dependent upon the user's code.
/// </summary>
/// <returns></returns>
public static JsonSerializerSettings GetOptions()
Copy link
Member

@serkantkaraca serkantkaraca Feb 23, 2018

Choose a reason for hiding this comment

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

JsonSerializerSettings [](start = 22, length = 22)

Allow custom settings as well. Like getter/setter, allow clients to customize their casing. If not set, return the default.

@serkantkaraca
Copy link
Member

Revert changes. Seems not related.


Refers to: src/Microsoft.Azure.EventHubs/Microsoft.Azure.EventHubs.csproj:1 in 6c7a1ea. [](commit_id = 6c7a1ea, deletion_comment = False)

@serkantkaraca
Copy link
Member

Revert changes. Seems not related.


Refers to: src/Microsoft.Azure.EventHubs.Processor/Microsoft.Azure.EventHubs.Processor.csproj:1 in 6c7a1ea. [](commit_id = 6c7a1ea, deletion_comment = False)

@serkantkaraca
Copy link
Member

serkantkaraca commented Feb 23, 2018

Did a small experiment. This is what most people have now due to DefaultContractResolver.
"{"Offset":null,"SequenceNumber":0,"PartitionId":"123","Owner":"this owner","Token":"this token","Epoch":0}"

This is what client generates after the change.
"{\r\n "sequenceNumber": 0,\r\n "partitionId": "123",\r\n "owner": "this owner",\r\n "token": "this token",\r\n "epoch": 0\r\n}"

We should keep the DefaultContractResolver as default and provide an API to customize settings. What do you think?

@BartJanvanAssen
Copy link
Author

We can skip this Formatting = Formatting.Indented, to resolve the (\r\n).

I think we should make it optional indeed. However maybe give them some options? camelcase and the default? and throw an exception when people enable other resolvers. Options depend on what the EH backend interface can cope with.

@serkantkaraca
Copy link
Member

The entire settings should be optional. It will be up to developer how to customize serialization.

Please look at the PartitionManagerOptions. I believe we can provide settings there.

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