{"action":"created","issue":{"url":"https://api.github.com/repos/confluentinc/ksql/issues/3354","repository_url":"https://api.github.com/repos/confluentinc/ksql","labels_url":"https://api.github.com/repos/confluentinc/ksql/issues/3354/labels{/name}","comments_url":"https://api.github.com/repos/confluentinc/ksql/issues/3354/comments","events_url":"https://api.github.com/repos/confluentinc/ksql/issues/3354/events","html_url":"https://github.com/confluentinc/ksql/pull/3354","id":493534329,"node_id":"MDExOlB1bGxSZXF1ZXN0MzE3NDk3MjQ4","number":3354,"title":"feat: update query id generation to use command topic record offset","user":{"login":"stevenpyzhang","id":35498506,"node_id":"MDQ6VXNlcjM1NDk4NTA2","avatar_url":"https://avatars1.githubusercontent.com/u/35498506?v=4","gravatar_id":"","url":"https://api.github.com/users/stevenpyzhang","html_url":"https://github.com/stevenpyzhang","followers_url":"https://api.github.com/users/stevenpyzhang/followers","following_url":"https://api.github.com/users/stevenpyzhang/following{/other_user}","gists_url":"https://api.github.com/users/stevenpyzhang/gists{/gist_id}","starred_url":"https://api.github.com/users/stevenpyzhang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stevenpyzhang/subscriptions","organizations_url":"https://api.github.com/users/stevenpyzhang/orgs","repos_url":"https://api.github.com/users/stevenpyzhang/repos","events_url":"https://api.github.com/users/stevenpyzhang/events{/privacy}","received_events_url":"https://api.github.com/users/stevenpyzhang/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2019-09-13T22:05:00Z","updated_at":"2019-09-19T13:30:42Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/confluentinc/ksql/pulls/3354","html_url":"https://github.com/confluentinc/ksql/pull/3354","diff_url":"https://github.com/confluentinc/ksql/pull/3354.diff","patch_url":"https://github.com/confluentinc/ksql/pull/3354.patch"},"body":"### Description \r\nFollow up to https://github.com/confluentinc/ksql/pull/3343/files\r\nUpdating query id generation code to utilize an offset value that's obtained from the corresponding Kafka record containing the query generating command.\r\n\r\nExisting query generating commands in the command topic need to maintain the old incremental query id generation.\r\n\r\nDeleted one of KsqlResourceFunctionalTests because the command topic is shared between multiple tests, so predicting the query id would be challenging because the offset for a command could potentially change.\r\n\r\n### Testing done \r\nLocal testing\r\nUpdate unit tests\r\nNew Unit tests\r\n### Reviewer checklist\r\n- [ ] Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).\r\n- [ ] Ensure relevant issues are linked (description should include text like \"Fixes #<issue number>\")\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/confluentinc/ksql/issues/comments/533131524","html_url":"https://github.com/confluentinc/ksql/pull/3354#issuecomment-533131524","issue_url":"https://api.github.com/repos/confluentinc/ksql/issues/3354","id":533131524,"node_id":"MDEyOklzc3VlQ29tbWVudDUzMzEzMTUyNA==","user":{"login":"big-andy-coates","id":8012398,"node_id":"MDQ6VXNlcjgwMTIzOTg=","avatar_url":"https://avatars0.githubusercontent.com/u/8012398?v=4","gravatar_id":"","url":"https://api.github.com/users/big-andy-coates","html_url":"https://github.com/big-andy-coates","followers_url":"https://api.github.com/users/big-andy-coates/followers","following_url":"https://api.github.com/users/big-andy-coates/following{/other_user}","gists_url":"https://api.github.com/users/big-andy-coates/gists{/gist_id}","starred_url":"https://api.github.com/users/big-andy-coates/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/big-andy-coates/subscriptions","organizations_url":"https://api.github.com/users/big-andy-coates/orgs","repos_url":"https://api.github.com/users/big-andy-coates/repos","events_url":"https://api.github.com/users/big-andy-coates/events{/privacy}","received_events_url":"https://api.github.com/users/big-andy-coates/received_events","type":"User","site_admin":false},"created_at":"2019-09-19T13:30:42Z","updated_at":"2019-09-19T13:30:42Z","author_association":"MEMBER","body":"> A new method could be added to KsqlEngine to return the QueryIdGenerator from the EngineContext so it can be updated\r\n\r\nRather than add accessors to get at the queryId generator, it would be better to leverage the fact we're injecting it.  If something else needs the generator, then it too should have it injected.\r\n\r\nI would make `QueryIdGenerator` an interface and add the following implementations:\r\n\r\n1. The existing implementation that generates ids based on a counter.\r\n1. A new implementation that generates ids based on the offset, (throwing if called twice).\r\n1. A composite generator that can have it's delegate generator switched between the two modes. Internally, it will have an instance of each of the above impls and methods to allow the active generator to be switched.\r\n\r\nThe new and old query id generators could use a different id prefix to ensure there is zero chance of a clash.\r\n\r\n### Standalone\r\nWhen constructing the engine in standalone mode we inject the existing implementation.\r\n\r\n### Interactive\r\nWhen constructing the engine in interactive mode we inject the composite generator.  \r\n\r\nThe `StatementExecutor` handles parsing the statement, so will know if a statement is a new or old style query.  It should have the engine's composite generator injected as a dependency and can then flip the generator to the right mode and set the offset.\r\n\r\nQuery validation happens on one of several rest handler threads.  These need to be able to take a snapshot of the engine and apply new statements to them in a sandboxed environment.   While validating several statements generated ids would need to be sequential, so the sandboxed engine actual needs the old style generator, (though maybe with a different prefix). The old style generator needs to have its current count initialized off the last offset applied to the engine before it was snapshotted. This will ensure the correct sequence of ids will be generated.  "}}
{"actor":{"display_login":"big-andy-coates"}}