{"action":"created","issue":{"url":"https://api.github.com/repos/LiveRamp/jack/issues/181","repository_url":"https://api.github.com/repos/LiveRamp/jack","labels_url":"https://api.github.com/repos/LiveRamp/jack/issues/181/labels{/name}","comments_url":"https://api.github.com/repos/LiveRamp/jack/issues/181/comments","events_url":"https://api.github.com/repos/LiveRamp/jack/issues/181/events","html_url":"https://github.com/LiveRamp/jack/pull/181","id":205415398,"number":181,"title":"Jack Transactor","user":{"login":"tuliren","id":1933157,"avatar_url":"https://avatars.githubusercontent.com/u/1933157?v=3","gravatar_id":"","url":"https://api.github.com/users/tuliren","html_url":"https://github.com/tuliren","followers_url":"https://api.github.com/users/tuliren/followers","following_url":"https://api.github.com/users/tuliren/following{/other_user}","gists_url":"https://api.github.com/users/tuliren/gists{/gist_id}","starred_url":"https://api.github.com/users/tuliren/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tuliren/subscriptions","organizations_url":"https://api.github.com/users/tuliren/orgs","repos_url":"https://api.github.com/users/tuliren/repos","events_url":"https://api.github.com/users/tuliren/events{/privacy}","received_events_url":"https://api.github.com/users/tuliren/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2017-02-05T10:13:41Z","updated_at":"2017-02-05T22:10:40Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/LiveRamp/jack/pulls/181","html_url":"https://github.com/LiveRamp/jack/pull/181","diff_url":"https://github.com/LiveRamp/jack/pull/181.diff","patch_url":"https://github.com/LiveRamp/jack/pull/181.patch"},"body":"## Introduction\r\nCurrently Jack has no mechanism for DB connection management. When the user is not careful (e.g. if `IDb` is wrapped inside a `ThreadLocal` object, and used in a multi-threaded process with large number of threads, which was what happened in SOA Refresher), SQL operations from Jack can consume a large amount of connections from the DB server.\r\n\r\nThis problem can be solved by using a capped DB connection pool. However, a resource pool is not enough. We also need to ensure that the connection is always returned to the pool when an operation is completed. (Of course this is assuming that users cannot be trusted.) This pull request attempts to achieve this goal with a new `Transactor` class, which executes SQL operations with automatic DB management.\r\n\r\nThis pull request is far from mergeable yet. I'd like to collect some feedbacks about the interface first.\r\n\r\n## SQL Operations\r\nTwo functional interfaces are created to specify SQL operations: `IQuery` and `IExecution`. The former has a return value and the latter does not:\r\n```java\r\n@FunctionalInterface\r\npublic interface IQuery<DB extends IDb, T> {\r\n  T query(DB db) throws IOException;\r\n}\r\n\r\n@FunctionalInterface\r\npublic interface IExecution<DB extends IDb> {\r\n  void execute(DB db) throws IOException;\r\n}\r\n```\r\n\r\n## Transactor\r\nA transactor runs SQL operations with a DB connection pool. There is one transactor for each `IDb`. It can be constructed as follows:\r\n```java\r\nITransactor<IDatabase1> transactor = new DatabasesImpl().getDatabase1Transactor()\r\n    .setMaxConnections(3)\r\n    .setTimeout(Duration.standardSeconds(5))\r\n    .get();\r\n```\r\n\r\nThe `maxConnection` specifies the max number of DB connections allowed. The `timeout` specifies that when no idle connection is available, how long a transaction should wait before it fails due to timeout.\r\n\r\n`Transactor` has four basic methods:\r\n```java\r\npublic interface ITransactor<DB extends IDb> extends Closeable {\r\n  <T> T query(IQuery<DB, T> query);\r\n  <T> T transQuery(IQuery<DB, T> query);\r\n  void execute(IExecution<DB> execution);\r\n  void transExecute(IExecution<DB> execution);\r\n}\r\n```\r\nThe `trans-` methods run operations in transaction.\r\n\r\nUsing Java 8 lambda, SQL operations can be executed as follows:\r\n```java\r\n// single line non-transactional query\r\nlong userId = transactor.query(db -> db.users().createDefaultInstance().getId());\r\n\r\n// multi-line transactional execution\r\ntransactor.transExecute(db -> {\r\n  db.users().find(userId).setNumPosts(111).save();\r\n  db.posts().createDefaultInstance().setUserId(userId);\r\n  db.comments().createDefaultInstance().setCommenterId(userId);\r\n});\r\n```\r\nWhen there is an exception, an unchecked exception `SqlExecutionFailureException` will be thrown out. Transactor does not throw a checked exception because there is usually not much to do about it, and the code is much cleaner this way. Users can still catch `SqlExecutionFailureException` when necessary.\r\n\r\nCommits and rollbacks will be taken care of in the `trans-` methods.\r\n\r\n## DB Management\r\nA DB manager inside the transactor is responsible for creating and allocating DB connections. The transactor gets and returns a connection from it before and after the execution of a SQL operation, respectively. Apart from a `FixedDbManager`, which keeps a capped DB connection pool, there is also a `FlexibleDbManager` which allows infinite number of DB connections, but it will wait for the duration of `timeout` before creating a new one. This implementation may be used for back-compatibility, or for investigation of proper number of max connections before migrating to `FixedDbManager`.\r\n\r\n## Todo\r\n- [ ] Make `TransactorImpl` a singleton.\r\n- [ ] Optimize DB connection management. Currently the idle DB connection list is synchronized with `wait` and `notify` methods. In this way, the operations to get and return connections are treated with equal priority. Preferably, return operations should be prioritized when there is no idle connections.\r\n- [ ] Create unit tests.\r\n- [ ] Optimize `TransactorImpl.Builder`.\r\n\r\n@pwestling, the previous approach we talked about actually does not work. The SQL exceptions cannot be caught to allow an automatic rollback. I think the current approach does not have that issue and is much cleaner.\r\n"},"comment":{"url":"https://api.github.com/repos/LiveRamp/jack/issues/comments/277553981","html_url":"https://github.com/LiveRamp/jack/pull/181#issuecomment-277553981","issue_url":"https://api.github.com/repos/LiveRamp/jack/issues/181","id":277553981,"user":{"login":"roshan","id":7322596,"avatar_url":"https://avatars.githubusercontent.com/u/7322596?v=3","gravatar_id":"","url":"https://api.github.com/users/roshan","html_url":"https://github.com/roshan","followers_url":"https://api.github.com/users/roshan/followers","following_url":"https://api.github.com/users/roshan/following{/other_user}","gists_url":"https://api.github.com/users/roshan/gists{/gist_id}","starred_url":"https://api.github.com/users/roshan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/roshan/subscriptions","organizations_url":"https://api.github.com/users/roshan/orgs","repos_url":"https://api.github.com/users/roshan/repos","events_url":"https://api.github.com/users/roshan/events{/privacy}","received_events_url":"https://api.github.com/users/roshan/received_events","type":"User","site_admin":false},"created_at":"2017-02-05T22:10:40Z","updated_at":"2017-02-05T22:10:40Z","body":"Nice!\r\n\r\nOne thing, it's one connection per thread, right? The MySQL JDBC driver (IIRC) is supposed to have a thread-safe `Connection` but actually locks the same two things in two different places in two different orders, making it susceptible to deadlocking (even on read-only).\r\n\r\nAs a minor detail, `transQuery` seems to imply read-only DB operations but there's no reason the `IQuery` has to be read-only. Any reason to distinguish the two operations `transQuery` and `transExecute` in name considering they perform the same 'thing' (run against the DB in a transaction)? There's enough info there for Java to disambiguate if they're both called the same thing. I know this is contrary to what @bpodgursky says but the intent of both commands is the same, and should I change the type I'm passing in from one to the other, I gain nothing from having to change the name of the method I'm calling."}}
{"actor":{"display_login":"roshan"}}