ignite-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vladimir Ozerov (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (IGNITE-7173) SQL: implement optional row cache
Date Wed, 27 Dec 2017 08:22:00 GMT

    [ https://issues.apache.org/jira/browse/IGNITE-7173?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16304342#comment-16304342
] 

Vladimir Ozerov edited comment on IGNITE-7173 at 12/27/17 8:21 AM:
-------------------------------------------------------------------

[~tledkov-gridgain], my comments:
1) {{H2RowCache.touch}} - is this the only possible approach to "touching" the page? The thing
is that when we hit the cache, page is already acquired, so probably there should be less
intrusive ways to do this. May be we even do not need to "touch" it at all. Please verify.
2) {{RowStore.ctor}} - please make sure that you do not hit NPE if indexing is not configured.
Also you will obviously get NPEs in other places if cache is disabled in cache configuration.
Moreover, some cache in the group may have
row cache, while others - don't. Again this would leave to NPE. Also, cache group context
is started *before* cache context. So I doubt it could work at all. Can we inject row cache
if it exists to row store inside cache start routine instead? Or may be it is better to do
a lookup on every update/remove call.
Please make sure to cover with tests all possible cases.
3) {{PageMemoryImpl}} - same as p.1, not sure we need change "touch" logic anyhow, as row
cache doesn't affect read lock/unlock cycles, so page usage stats should be updated anyway.
4) {{H2RowCachePageEvictionTest._testTouchPageOnRead}} - dead code
5) We need more precise tests. It is not enough to simply check that cache size changed. In
case of update/remove we need to verify that exactly updated entries were removed. This can
be done by iether manual inspection of row cache content, or indirectly through new query.
6) {{CacheConfiguration}} - we need better JavaDocs for this feature, describing on when and
how entries are cached and invalidated.
7) Initial implementation also contained changes to {{CacheQueryObjectValueContext}} - {{copyOnGet}}
was changed to {{false}} and {{storeValue}} was changed to {{true}}. This was done for a reason
- in this mode when binary object is cached inside a row and SqlQuery is executed, then deserialized
value is cached in inside binary object as well, what increases SqlQuery speed significantly.
Any reason why these changes were reverted?


was (Author: vozerov):
[~tledkov-gridgain], my comments:
1) H2RowCache.touch - is this the only possible approach to "touching" the page? The thing
is that when we hit the cache, page is already acquired, so probably there should be more
performant ways to do this. May be we even do not need to "touch" it at all. Please verify.
2) RowStore.ctor - please make sure that you do not hit NPE if indexing is not configured.
Also you will obviously get NPEs in other places if cache is disabled in cache configuration.
Moreover, some cache in the group may have
row cache, while others - don't. Again this would leave to NPE. Also, cache group context
is started *before* cache context. So I doubt it could work at all. Can we inject row cache
if it exists to row store inside cache start routine instead? Or may be it is better to do
a lookup on every update/remove call.
Please make sure to cover with tests all possible cases.
3) PageMemoryImpl - same as p.1, not sure we need change "touch" logic anyhow, as row cache
doesn't affect read lock/unlock cycles, so page usage stats should be updated anyway.
4) H2RowCachePageEvictionTest._testTouchPageOnRead - dead code
5) We need more precise tests. It is not enough to simply check that cache size changed. In
case of update/remove we need to verify that exactly updated entries were removed. This can
be done by iether manual inspection of row cache content, or indirectly through new query.
6) CacheConfiguration - we need better JavaDocs for this feature, describing on when and how
entries are cached and invalidated.
7) Initial implementation also contained changes to {{CacheQueryObjectValueContext}} - {{copyOnGet}}
was changed to {{false}} and {{storeValue}} was changed to {{true}}. This was done for a reason
- in this mode when binary object is cached inside a row and SqlQuery is executed, then deserialized
value is cached in inside binary object as well, what increases SqlQuery speed significantly.
Any reason why these changes were reverted?

> SQL: implement optional row cache
> ---------------------------------
>
>                 Key: IGNITE-7173
>                 URL: https://issues.apache.org/jira/browse/IGNITE-7173
>             Project: Ignite
>          Issue Type: Bug
>          Components: sql
>            Reporter: Vladimir Ozerov
>            Assignee: Taras Ledkov
>             Fix For: 2.4
>
>




--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

Mime
View raw message