{"action":"created","issue":{"url":"https://api.github.com/repos/grpc/grpc/issues/14483","repository_url":"https://api.github.com/repos/grpc/grpc","labels_url":"https://api.github.com/repos/grpc/grpc/issues/14483/labels{/name}","comments_url":"https://api.github.com/repos/grpc/grpc/issues/14483/comments","events_url":"https://api.github.com/repos/grpc/grpc/issues/14483/events","html_url":"https://github.com/grpc/grpc/pull/14483","id":299213113,"number":14483,"title":"[grpc] Add SSL session client cache support","user":{"login":"euroelessar","id":83361,"avatar_url":"https://avatars1.githubusercontent.com/u/83361?v=4","gravatar_id":"","url":"https://api.github.com/users/euroelessar","html_url":"https://github.com/euroelessar","followers_url":"https://api.github.com/users/euroelessar/followers","following_url":"https://api.github.com/users/euroelessar/following{/other_user}","gists_url":"https://api.github.com/users/euroelessar/gists{/gist_id}","starred_url":"https://api.github.com/users/euroelessar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/euroelessar/subscriptions","organizations_url":"https://api.github.com/users/euroelessar/orgs","repos_url":"https://api.github.com/users/euroelessar/repos","events_url":"https://api.github.com/users/euroelessar/events{/privacy}","received_events_url":"https://api.github.com/users/euroelessar/received_events","type":"User","site_admin":false},"labels":[{"id":166332702,"url":"https://api.github.com/repos/grpc/grpc/labels/area/core","name":"area/core","color":"efdb40","default":false},{"id":170043423,"url":"https://api.github.com/repos/grpc/grpc/labels/area/security","name":"area/security","color":"efdb40","default":false}],"state":"open","locked":false,"assignee":{"login":"jiangtaoli2016","id":19913700,"avatar_url":"https://avatars2.githubusercontent.com/u/19913700?v=4","gravatar_id":"","url":"https://api.github.com/users/jiangtaoli2016","html_url":"https://github.com/jiangtaoli2016","followers_url":"https://api.github.com/users/jiangtaoli2016/followers","following_url":"https://api.github.com/users/jiangtaoli2016/following{/other_user}","gists_url":"https://api.github.com/users/jiangtaoli2016/gists{/gist_id}","starred_url":"https://api.github.com/users/jiangtaoli2016/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jiangtaoli2016/subscriptions","organizations_url":"https://api.github.com/users/jiangtaoli2016/orgs","repos_url":"https://api.github.com/users/jiangtaoli2016/repos","events_url":"https://api.github.com/users/jiangtaoli2016/events{/privacy}","received_events_url":"https://api.github.com/users/jiangtaoli2016/received_events","type":"User","site_admin":false},"assignees":[{"login":"jiangtaoli2016","id":19913700,"avatar_url":"https://avatars2.githubusercontent.com/u/19913700?v=4","gravatar_id":"","url":"https://api.github.com/users/jiangtaoli2016","html_url":"https://github.com/jiangtaoli2016","followers_url":"https://api.github.com/users/jiangtaoli2016/followers","following_url":"https://api.github.com/users/jiangtaoli2016/following{/other_user}","gists_url":"https://api.github.com/users/jiangtaoli2016/gists{/gist_id}","starred_url":"https://api.github.com/users/jiangtaoli2016/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jiangtaoli2016/subscriptions","organizations_url":"https://api.github.com/users/jiangtaoli2016/orgs","repos_url":"https://api.github.com/users/jiangtaoli2016/repos","events_url":"https://api.github.com/users/jiangtaoli2016/events{/privacy}","received_events_url":"https://api.github.com/users/jiangtaoli2016/received_events","type":"User","site_admin":false}],"milestone":null,"comments":61,"created_at":"2018-02-22T03:51:28Z","updated_at":"2018-03-16T07:10:19Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/grpc/grpc/pulls/14483","html_url":"https://github.com/grpc/grpc/pull/14483","diff_url":"https://github.com/grpc/grpc/pull/14483.diff","patch_url":"https://github.com/grpc/grpc/pull/14483.patch"},"body":"grpc-core part of #14425 \r\n\r\nProvide caller ability to specify cache for reusable sessions.\r\nAdd LRU-based cache implementation and expose its API publicly.\r\n\r\nCover cache by unit tests and ticket resumption logic by end-to-end integration tests.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/grpc/grpc/14483)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/grpc/grpc/issues/comments/373623095","html_url":"https://github.com/grpc/grpc/pull/14483#issuecomment-373623095","issue_url":"https://api.github.com/repos/grpc/grpc/issues/14483","id":373623095,"user":{"login":"euroelessar","id":83361,"avatar_url":"https://avatars1.githubusercontent.com/u/83361?v=4","gravatar_id":"","url":"https://api.github.com/users/euroelessar","html_url":"https://github.com/euroelessar","followers_url":"https://api.github.com/users/euroelessar/followers","following_url":"https://api.github.com/users/euroelessar/following{/other_user}","gists_url":"https://api.github.com/users/euroelessar/gists{/gist_id}","starred_url":"https://api.github.com/users/euroelessar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/euroelessar/subscriptions","organizations_url":"https://api.github.com/users/euroelessar/orgs","repos_url":"https://api.github.com/users/euroelessar/repos","events_url":"https://api.github.com/users/euroelessar/events{/privacy}","received_events_url":"https://api.github.com/users/euroelessar/received_events","type":"User","site_admin":false},"created_at":"2018-03-16T07:10:19Z","updated_at":"2018-03-16T07:10:19Z","author_association":"CONTRIBUTOR","body":"Thanks, I have couple of question and left them inline.  I've also addressed other concerns.\n\n---\n\nReview status: all files reviewed at latest revision, 15 unresolved discussions.\n\n---\n\n*[src/core/tsi/ssl_session.h, line 54 at r3](https://reviewable.io:443/reviews/grpc/grpc/14483#-L7eMjJoyMDNumYdXpya:-L7hURyw_YV5KA6qXYMf:bs065ff) ([raw file](https://github.com/grpc/grpc/blob/99589d619df1fddc4beb19acac9416408ab1548c/src/core/tsi/ssl_session.h#L54)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nPlease document this class.  What does it represent?  How is it expected to be used?\n\nThe API is not at all obvious here.  Does instantiating this class cause the session to be cached?  If not, what does it do?  If so, why does the `Get()` method return an owned pointer?  What happens to the `SslCachedSession` object once `Get()` is called, since it no longer owns the underlying session?\n</blockquote></details>\n\nI've added more documentation and changed naming of `Get` function, please let me know if it needs more clarification or changes\n\n---\n\n*[src/core/tsi/ssl_session_cache.h, line 105 at r2](https://reviewable.io:443/reviews/grpc/grpc/14483#-L7CIPBjLgXejkv9CwhN:-L7gFLdejhl8M19LEc7z:b-3hwsbx) ([raw file](https://github.com/grpc/grpc/blob/07e89d5889ed2d212fed7809d2523d33d65639cf/src/core/tsi/ssl_session_cache.h#L105)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nI'm not suggesting completely replacing `SslSessionLRUCache` with `SliceWeakHashTable`; I'm just suggesting replacing the use of `gpr_avl` with `SliceWeakHashTable`.  \n\nWe can still provide LRU semantics here.  I realize that `SliceWeakHashTable` has the semantic that old entries get removed on hash table collisions, but I would think that you could avoid those collisions by setting the hash key appropriately.  The idea would be to basically just use `SliceWeakHashTable` as a mutable hash table (similar to `SliceHashTable`, except that that class is immutable).\n\nIn any case, there's no real problem with using `gpr_avl` if you want to stick with it.\n</blockquote></details>\n\nI need pick good hash and pick table's size (template compile-time argument) based on the desired capacity (dynamic application-specific value). Also it would require exposing stats into application layer about cache eviction to be able detect bad scenarios in production caused by bad hash function or unlucky identities naming..\n\nThanks for the suggestion but avl (or, better, non-weak hash table) fits better here.\n\n---\n\n*[src/core/tsi/ssl_transport_security.cc, line 84 at r3](https://reviewable.io:443/reviews/grpc/grpc/14483#-L7eU5NeD18NdF09gmaF:-L7fbTcTS85OjyZvT3Kb:b5kotiy) ([raw file](https://github.com/grpc/grpc/blob/99589d619df1fddc4beb19acac9416408ab1548c/src/core/tsi/ssl_transport_security.cc#L84)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nPlease use `RefCountedPtr<>` here.\n</blockquote></details>\n\nCan I ask what is a recommendation about placing C++ objects in managed by C structures please? (by malloc, memset and free)\nI'm not sure that refactoring tsi into C++ fits into scope of this change.\n\n---\n\n*[src/core/tsi/ssl_transport_security.cc, line 118 at r3](https://reviewable.io:443/reviews/grpc/grpc/14483#-L7eTWJF1FcOKyMo6dBF:-L7fbPbJ7pz2c7gUau6o:bsslprw) ([raw file](https://github.com/grpc/grpc/blob/99589d619df1fddc4beb19acac9416408ab1548c/src/core/tsi/ssl_transport_security.cc#L118)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nOur convention is that global variables should start with the prefix `g_`.\n</blockquote></details>\n\nShould I change other variables in the same CL?\n\n---\n\n*[test/core/tsi/ssl_transport_security_test.cc, line 79 at r3](https://reviewable.io:443/reviews/grpc/grpc/14483#-L7f5O0ZUVn4i-WtOEcE:-L7gF0JT6arbj0pouNBF:b-wgprvl) ([raw file](https://github.com/grpc/grpc/blob/99589d619df1fddc4beb19acac9416408ab1548c/test/core/tsi/ssl_transport_security_test.cc#L79)):*\n<details><summary><i>Previously, markdroth (Mark D. Roth) wrote…</i></summary><blockquote>\n\nPlease use `RefCountedPtr<>`.\n</blockquote></details>\n\nSame question here as for tsi.. Is there a recommendation for storing C++ classes in C-managed structs or should I refactor it in this CL?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/grpc/grpc/14483#-:-L7hd1vPcmqj15zP7UGi:b-f30i2f)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"euroelessar"}}