{"action":"created","issue":{"url":"https://api.github.com/repos/confluentinc/confluent-kafka-dotnet/issues/202","repository_url":"https://api.github.com/repos/confluentinc/confluent-kafka-dotnet","labels_url":"https://api.github.com/repos/confluentinc/confluent-kafka-dotnet/issues/202/labels{/name}","comments_url":"https://api.github.com/repos/confluentinc/confluent-kafka-dotnet/issues/202/comments","events_url":"https://api.github.com/repos/confluentinc/confluent-kafka-dotnet/issues/202/events","html_url":"https://github.com/confluentinc/confluent-kafka-dotnet/pull/202","id":231880729,"number":202,"title":"reworking serialization API for schema registry support","user":{"login":"mhowlett","id":19174,"avatar_url":"https://avatars1.githubusercontent.com/u/19174?v=3","gravatar_id":"","url":"https://api.github.com/users/mhowlett","html_url":"https://github.com/mhowlett","followers_url":"https://api.github.com/users/mhowlett/followers","following_url":"https://api.github.com/users/mhowlett/following{/other_user}","gists_url":"https://api.github.com/users/mhowlett/gists{/gist_id}","starred_url":"https://api.github.com/users/mhowlett/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mhowlett/subscriptions","organizations_url":"https://api.github.com/users/mhowlett/orgs","repos_url":"https://api.github.com/users/mhowlett/repos","events_url":"https://api.github.com/users/mhowlett/events{/privacy}","received_events_url":"https://api.github.com/users/mhowlett/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2017-05-28T16:58:48Z","updated_at":"2017-05-29T09:04:35Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/confluentinc/confluent-kafka-dotnet/pulls/202","html_url":"https://github.com/confluentinc/confluent-kafka-dotnet/pull/202","diff_url":"https://github.com/confluentinc/confluent-kafka-dotnet/pull/202.diff","patch_url":"https://github.com/confluentinc/confluent-kafka-dotnet/pull/202.patch"},"body":"this is a continuation of @treziac 's #129. \r\n\r\nI think I finally have a definite opinion on changes that should be made to (de)serializers for schema registry support (a new idea).\r\n\r\n1. configuration works differently to java. the idea is a (de)serializer can take the configuration dict in their constructors, and the constructor of Producer/Consumer then ignores any properties used by the serializers. I also added capability for key and value serializers to be configured separately. we can update the string, int and long serializers to also optionally be configured from the main config dict. we can do the same architectural pattern for custom partitioners (allow them to be configured). \r\n2. no `IDisposable` - I don't see when this would be necessary. Only suggested it because it's there in java - what am i missing? we can have this if needed, but this raises questions of ownership - it's most convenient if the `Producer` and `Consumer` take ownership of the (de)serializers, and this is not without precedent in dotnet (see StreamReader), but it's a pattern that can cause confusion (in general, though probably not in our case) and best avoided. \r\n3. actual serializers and custom partitioner classes are not going in the configuration dict - they will be only arguments to the constructor. they can however be parameterized by values from the config. I think this is an optimal balance.\r\n4. no explicit implementation of interfaces (for reasons noted in #129).\r\n\r\nto use: \r\n\r\n```\r\nusing (var producer = new Producer<string, string>(config, new StringSerializer(config), new AvroSerializer(config)))\r\n{\r\n}\r\n```\r\n\r\n(the old way of configuring string serializer will remain as well).\r\n\r\nnote: WIP tests not updated, just putting the code changes out for comment. \r\n"},"comment":{"url":"https://api.github.com/repos/confluentinc/confluent-kafka-dotnet/issues/comments/304611860","html_url":"https://github.com/confluentinc/confluent-kafka-dotnet/pull/202#issuecomment-304611860","issue_url":"https://api.github.com/repos/confluentinc/confluent-kafka-dotnet/issues/202","id":304611860,"user":{"login":"treziac","id":2729252,"avatar_url":"https://avatars3.githubusercontent.com/u/2729252?v=3","gravatar_id":"","url":"https://api.github.com/users/treziac","html_url":"https://github.com/treziac","followers_url":"https://api.github.com/users/treziac/followers","following_url":"https://api.github.com/users/treziac/following{/other_user}","gists_url":"https://api.github.com/users/treziac/gists{/gist_id}","starred_url":"https://api.github.com/users/treziac/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/treziac/subscriptions","organizations_url":"https://api.github.com/users/treziac/orgs","repos_url":"https://api.github.com/users/treziac/repos","events_url":"https://api.github.com/users/treziac/events{/privacy}","received_events_url":"https://api.github.com/users/treziac/received_events","type":"User","site_admin":false},"created_at":"2017-05-29T09:04:35Z","updated_at":"2017-05-29T09:04:35Z","body":"1. Perhaps we should rater add the possibility to have config not used in librdkafka (and generate warnings like in java) rather than forbiding unknown conf (so we won't have to do the parsing to remove unknown config). No definitive opinion here...\r\n2. Yep, I just implemented it since you asked ;) I also prefer the user to take care of it if needed rather than giving the control to the handle and having potential bug.\r\n3. For the StringSerializer for example, Configuration would contain encoding, that's it? And all serializer would preferably have a constructor with a IEnumerable<KeyValuePair<String, object>>? In this case, we could also add a (facultative) base class wich deal with this automatically.\r\n4. With this configuration object, we can't know the type of config object (for librdkafka as it's an external library it's not such an issue, here it's a bit more problematic) I would rather have properties, and have the base constructor I mentioned before do the mapping (so both can be used, constructor with initialization and/or constructor with config - I never liked initializing things at runtime (dictionary) rather than compile timpe (properties), I assume I'm not the only C# dev in this case\r\n\r\nPerhaps add the StringSerializer with encoding as config to show exactly what you want to do\r\n\r\n5. This may come later, but I see nothing which would prevent specifying the serializer/deserializer/partitioner via config also with this. For people who like everything in config (very useful when done via file without having to look at code with miscroservices and co, I must admit ;) )"}}
{"actor":{"display_login":"treziac"}}