{"action":"created","issue":{"url":"https://api.github.com/repos/pinterest/secor/issues/450","repository_url":"https://api.github.com/repos/pinterest/secor","labels_url":"https://api.github.com/repos/pinterest/secor/issues/450/labels{/name}","comments_url":"https://api.github.com/repos/pinterest/secor/issues/450/comments","events_url":"https://api.github.com/repos/pinterest/secor/issues/450/events","html_url":"https://github.com/pinterest/secor/pull/450","id":355739504,"node_id":"MDExOlB1bGxSZXF1ZXN0MjEyMTk5NTE2","number":450,"title":"Kafka 2.0.0 Client","user":{"login":"igorcalabria","id":2111602,"node_id":"MDQ6VXNlcjIxMTE2MDI=","avatar_url":"https://avatars3.githubusercontent.com/u/2111602?v=4","gravatar_id":"","url":"https://api.github.com/users/igorcalabria","html_url":"https://github.com/igorcalabria","followers_url":"https://api.github.com/users/igorcalabria/followers","following_url":"https://api.github.com/users/igorcalabria/following{/other_user}","gists_url":"https://api.github.com/users/igorcalabria/gists{/gist_id}","starred_url":"https://api.github.com/users/igorcalabria/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/igorcalabria/subscriptions","organizations_url":"https://api.github.com/users/igorcalabria/orgs","repos_url":"https://api.github.com/users/igorcalabria/repos","events_url":"https://api.github.com/users/igorcalabria/events{/privacy}","received_events_url":"https://api.github.com/users/igorcalabria/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":22,"created_at":"2018-08-30T20:21:40Z","updated_at":"2018-11-08T08:35:43Z","closed_at":"2018-11-05T22:22:39Z","author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/pinterest/secor/pulls/450","html_url":"https://github.com/pinterest/secor/pull/450","diff_url":"https://github.com/pinterest/secor/pull/450.diff","patch_url":"https://github.com/pinterest/secor/pull/450.patch"},"body":"https://github.com/pinterest/secor/issues/397 https://github.com/pinterest/secor/issues/393\r\nI wasn't sure If maintaining compatibility between all kafka versions was desired but I figured that we can't simply change a bunch of stuff without giving people some time to adjust. Release profile is still set to 0.10.2. \r\n\r\nIt is important to highlight some things:\r\n- New kafka client lib does not include the broker, so we need another way to run it. I used `docker-compose` because it was easier for me and it made some sense because the project already supports docker. This is not set in stone and we can change that\r\n\r\n- I don't think we'll be able to support committing to kafka with the new client. After a rebalance, kafka will throw `CommitFailedException` if you try to commit a rebalanced TopicPartition. These topic partitions will be assigned to another instance but `commit()` will never be called in those instances since secor will skip already uploaded files(locked via zookeeper). Because of this, I'm simply reading the offsets from zookeeper.\r\n\r\n- Travis will fail because I haven't configured docker-compose for it. We need to agree on a way to run kafka in the integration tests\r\n\r\n- `KafkaMessageIterator` interface fits pretty weirdly with the new consumer's `poll` method. I Chose this approach because it minimizes the changes I had to do to the old code. \r\n\r\n- I haven't added all new consumer configs to `SecorConfig`. \"low importance\" ones were skipped https://kafka.apache.org/documentation/#configuration\r\n\r\n- Performance test and production validation. I'll deploy a production copy in our environment and see how the new consumer behaves. I'll try to force rebalances as much as possible. It would be great if more people ran something similar."},"comment":{"url":"https://api.github.com/repos/pinterest/secor/issues/comments/436914329","html_url":"https://github.com/pinterest/secor/pull/450#issuecomment-436914329","issue_url":"https://api.github.com/repos/pinterest/secor/issues/450","id":436914329,"node_id":"MDEyOklzc3VlQ29tbWVudDQzNjkxNDMyOQ==","user":{"login":"juhoautio","id":4446608,"node_id":"MDQ6VXNlcjQ0NDY2MDg=","avatar_url":"https://avatars3.githubusercontent.com/u/4446608?v=4","gravatar_id":"","url":"https://api.github.com/users/juhoautio","html_url":"https://github.com/juhoautio","followers_url":"https://api.github.com/users/juhoautio/followers","following_url":"https://api.github.com/users/juhoautio/following{/other_user}","gists_url":"https://api.github.com/users/juhoautio/gists{/gist_id}","starred_url":"https://api.github.com/users/juhoautio/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/juhoautio/subscriptions","organizations_url":"https://api.github.com/users/juhoautio/orgs","repos_url":"https://api.github.com/users/juhoautio/repos","events_url":"https://api.github.com/users/juhoautio/events{/privacy}","received_events_url":"https://api.github.com/users/juhoautio/received_events","type":"User","site_admin":false},"created_at":"2018-11-08T08:35:43Z","updated_at":"2018-11-08T08:35:43Z","author_association":"NONE","body":"Actually no. I didn't realize that the fixes only target the kafka-2.0.0 profile. Why so?\r\n\r\nWe use kafka-1.0.0 profile where SecorKafkaMessageIterator is not available. It still has the same bug for kafka commit, it ignores the partition & offset:\r\nhttps://github.com/pinterest/secor/blob/b97bfded179e0a99b907eb989b9e0e3deaed5b40/src/main/java/com/pinterest/secor/reader/LegacyKafkaMessageIterator.java#L77\r\n\r\nI don't think we're upgrading to 2.0.0 very soon. But why not have at least the offset commit fix also in LegacyKafkaMessageIterator? Were there any other crucial fixes for the dual commit setup?"}}
{"actor":{"display_login":"juhoautio"}}