[
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)
