{"action":"created","issue":{"url":"https://api.github.com/repos/palantir/atlasdb/issues/1195","repository_url":"https://api.github.com/repos/palantir/atlasdb","labels_url":"https://api.github.com/repos/palantir/atlasdb/issues/1195/labels{/name}","comments_url":"https://api.github.com/repos/palantir/atlasdb/issues/1195/comments","events_url":"https://api.github.com/repos/palantir/atlasdb/issues/1195/events","html_url":"https://github.com/palantir/atlasdb/issues/1195","id":188669130,"number":1195,"title":"Oracle Table Name Mapping Perf","user":{"login":"clockfort","id":57031,"avatar_url":"https://avatars.githubusercontent.com/u/57031?v=3","gravatar_id":"","url":"https://api.github.com/users/clockfort","html_url":"https://github.com/clockfort","followers_url":"https://api.github.com/users/clockfort/followers","following_url":"https://api.github.com/users/clockfort/following{/other_user}","gists_url":"https://api.github.com/users/clockfort/gists{/gist_id}","starred_url":"https://api.github.com/users/clockfort/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/clockfort/subscriptions","organizations_url":"https://api.github.com/users/clockfort/orgs","repos_url":"https://api.github.com/users/clockfort/repos","events_url":"https://api.github.com/users/clockfort/events{/privacy}","received_events_url":"https://api.github.com/users/clockfort/received_events","type":"User","site_admin":false},"labels":[{"id":408238104,"url":"https://api.github.com/repos/palantir/atlasdb/labels/component:%20performance","name":"component: performance","color":"bfdadc","default":false},{"id":378357486,"url":"https://api.github.com/repos/palantir/atlasdb/labels/priority:%20P1","name":"priority: P1","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2016-11-11T02:15:47Z","updated_at":"2016-11-14T21:41:12Z","closed_at":null,"body":"It seems like the following codepath hits a part of the new oracle table name mapping magics that should be behind a cache/supplier, but appears not to be; if it is indeed not, it is a sizable perf regression.\r\n\r\nSnippet of my stacktrace:\r\n```\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.ConnectionSupplier.getFresh(ConnectionSupplier.java:49) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.ConnectionSupplier.get(ConnectionSupplier.java:38) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.OracleTableNameUnmapper.getShortTableNameFromMappingTable(OracleTableNameUnmapper.java:37) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.OracleTableNameGetter.getInternalShortTableName(OracleTableNameGetter.java:58) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.PrefixedTableNames.getOraclePrefixedTableName(PrefixedTableNames.java:45) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.PrefixedTableNames.get(PrefixedTableNames.java:36) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.ranges.DbKvsGetRanges.getPrefixedTableName(DbKvsGetRanges.java:333) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.ranges.DbKvsGetRanges.getSimpleRowSelectOneQueryOracle(DbKvsGetRanges.java:326) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.ranges.DbKvsGetRanges.getRangeQueryAndArgs(DbKvsGetRanges.java:199) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.ranges.DbKvsGetRanges.getFirstPages(DbKvsGetRanges.java:107) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.ranges.DbKvsGetRanges.getFirstBatchForRanges(DbKvsGetRanges.java:94) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.dbkvs.impl.DbKvs.getFirstBatchForRanges(DbKvs.java:384) ~[atlasdb-dbkvs/:na]\r\n\tat com.palantir.atlasdb.keyvalue.impl.ForwardingKeyValueService.getFirstBatchForRanges(ForwardingKeyValueService.java:129) ~[atlasdb-client/:na]\r\n```"},"comment":{"url":"https://api.github.com/repos/palantir/atlasdb/issues/comments/260471602","html_url":"https://github.com/palantir/atlasdb/issues/1195#issuecomment-260471602","issue_url":"https://api.github.com/repos/palantir/atlasdb/issues/1195","id":260471602,"user":{"login":"clockfort","id":57031,"avatar_url":"https://avatars.githubusercontent.com/u/57031?v=3","gravatar_id":"","url":"https://api.github.com/users/clockfort","html_url":"https://github.com/clockfort","followers_url":"https://api.github.com/users/clockfort/followers","following_url":"https://api.github.com/users/clockfort/following{/other_user}","gists_url":"https://api.github.com/users/clockfort/gists{/gist_id}","starred_url":"https://api.github.com/users/clockfort/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/clockfort/subscriptions","organizations_url":"https://api.github.com/users/clockfort/orgs","repos_url":"https://api.github.com/users/clockfort/repos","events_url":"https://api.github.com/users/clockfort/events{/privacy}","received_events_url":"https://api.github.com/users/clockfort/received_events","type":"User","site_admin":false},"created_at":"2016-11-14T21:41:12Z","updated_at":"2016-11-14T21:41:12Z","body":"Right now, the code causes anyone who calls getFirstBatchForRanges to get thousands of error messages from CloseTracking.java because the connection that it gets is closed on deref rather than manually.\r\n\r\nI think the easiest place to see the issue is here (I added two comments inline):\r\n\r\n```\r\n    private Map<RangeRequest, TokenBackedBasicResultsPage<RowResult<Value>, byte[]>> getFirstPages(\r\n            TableReference tableRef,\r\n            List<RangeRequest> requests,\r\n            long timestamp) {\r\n        List<String> subQueries = Lists.newArrayList();\r\n        List<Object> argsList = Lists.newArrayList();\r\n        for (int i = 0; i < requests.size(); i++) {\r\n            RangeRequest request = requests.get(i);\r\n            Pair<String, List<Object>> queryAndArgs = getRangeQueryAndArgs( // <-- this is the codepath we're in that opens a connection in the table mapper and doesn't close it before CloseTracking.java notices it's dereferenced\r\n                    tableRef,\r\n                    request.getStartInclusive(),\r\n                    request.getEndExclusive(),\r\n                    request.isReverse(),\r\n                    request.getBatchHint() == null ? 1 : request.getBatchHint(),\r\n                    i);\r\n            subQueries.add(queryAndArgs.lhSide);\r\n            argsList.addAll(queryAndArgs.rhSide);\r\n        }\r\n        String query = Joiner.on(\") UNION ALL (\").appendTo(new StringBuilder(\"(\"), subQueries).append(\")\").toString();\r\n        Object[] args = argsList.toArray();\r\n\r\n        TimingState timer = logTimer.begin(\"Table: \" + tableRef.getQualifiedName() + \" get_page\");\r\n        final SqlConnection conn = connectionSupplier.get();\r\n        try {\r\n            return getFirstPagesFromDb(tableRef, requests, timestamp, conn, query, args); // <-- this is the normal codepath that opens a connection and does close it manually / correctly\r\n        } finally {\r\n            closeSql(conn);\r\n            timer.end();\r\n        }\r\n    }\r\n```"}}
{"actor":{"display_login":"clockfort"}}