New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-implement cache according to Cache PSR #879

Closed
samdark opened this Issue Sep 15, 2013 · 21 comments

Comments

Projects
None yet
@samdark
Member

samdark commented Sep 15, 2013

Cache PSR is still in discussion stage but will be accepted at some point. In order to support PHP interoperability we may re-implement our caching solution according to the upcoming standard.

https://github.com/Crell/fig-standards/blob/2af1a03511706c32c056b4252de3704c674d4baa/proposed/cache-meta.md
https://github.com/Crell/fig-standards/blob/2af1a03511706c32c056b4252de3704c674d4baa/proposed/cache.md

@ghost ghost assigned samdark Sep 15, 2013

@pmoust

This comment has been minimized.

Show comment
Hide comment
@pmoust

pmoust Sep 15, 2013

Contributor

Let's follow it by all means.

If we come to an agreement, I could take the task and make a PR asap.

Contributor

pmoust commented Sep 15, 2013

Let's follow it by all means.

If we come to an agreement, I could take the task and make a PR asap.

@pmoust

This comment has been minimized.

Show comment
Hide comment
@pmoust

pmoust Sep 15, 2013

Contributor

Or you of course, just saw you had yourself assigned.

Contributor

pmoust commented Sep 15, 2013

Or you of course, just saw you had yourself assigned.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Sep 15, 2013

Member

This PSR proposal looks like over complicated to me. Not sure if we really want to change our API to that structure...

Member

cebe commented Sep 15, 2013

This PSR proposal looks like over complicated to me. Not sure if we really want to change our API to that structure...

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 15, 2013

Member

@cebe the document itself, same as any standard, looks complicated but the idea itself is OK and the syntax is not really complicated:

public function actionView($postID)
{
  $item = Yii::$app->cache->getItem('post_'.$postID);
  if (!$item->isHit()) {
    $post = Post::find($postID);
    $item->set($post);
  }
  $post = $item->get();

  return $this->render('view', array('post' => $post));
}

Current syntax:

public function actionView($postID)
{
  $cacheKey = 'post_'.$postID;
  $post = Yii::$app->cache->get($cacheKey);
  if ($post === false) {
    $post = Post::find($postID);
    Yii::$app->cache->set($cacheKey, $post);
  }

  return $this->render('view', array('post' => $post));
}
Member

samdark commented Sep 15, 2013

@cebe the document itself, same as any standard, looks complicated but the idea itself is OK and the syntax is not really complicated:

public function actionView($postID)
{
  $item = Yii::$app->cache->getItem('post_'.$postID);
  if (!$item->isHit()) {
    $post = Post::find($postID);
    $item->set($post);
  }
  $post = $item->get();

  return $this->render('view', array('post' => $post));
}

Current syntax:

public function actionView($postID)
{
  $cacheKey = 'post_'.$postID;
  $post = Yii::$app->cache->get($cacheKey);
  if ($post === false) {
    $post = Post::find($postID);
    Yii::$app->cache->set($cacheKey, $post);
  }

  return $this->render('view', array('post' => $post));
}
@shoaibi

This comment has been minimized.

Show comment
Hide comment
@shoaibi

shoaibi commented Sep 15, 2013

+1

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 16, 2013

Member

I can't say I support it 100%. Technically it's well thought but our caching solution proved to be sufficient and simpler so I'd like to get objective pros and cons from the community.

Member

samdark commented Sep 16, 2013

I can't say I support it 100%. Technically it's well thought but our caching solution proved to be sufficient and simpler so I'd like to get objective pros and cons from the community.

@shoaibi

This comment has been minimized.

Show comment
Hide comment
@shoaibi

shoaibi Sep 16, 2013

I am in support because even though the current caching solution has proved to be sufficient this seems to make more sense to me, and it appears somewhat cleaner, let alone it being a rfc. This makes more sense because it matches to the concepts of Cache Hit, Cache Miss that I learnt long, long, long ago.

shoaibi commented Sep 16, 2013

I am in support because even though the current caching solution has proved to be sufficient this seems to make more sense to me, and it appears somewhat cleaner, let alone it being a rfc. This makes more sense because it matches to the concepts of Cache Hit, Cache Miss that I learnt long, long, long ago.

@luislobo

This comment has been minimized.

Show comment
Hide comment
@luislobo

luislobo Sep 21, 2013

I support your idea. I really like the is hit method.

luislobo commented Sep 21, 2013

I support your idea. I really like the is hit method.

@FrediL

This comment has been minimized.

Show comment
Hide comment
@FrediL

FrediL Sep 21, 2013

Contributor

It looks like with right implementation it can overcome problem with caching false value.

Contributor

FrediL commented Sep 21, 2013

It looks like with right implementation it can overcome problem with caching false value.

@plandem

This comment has been minimized.

Show comment
Hide comment
@plandem

plandem Sep 24, 2013

well..this cache is ok for non highload, coz not atomic code.

$item = Yii::$app->cache->getItem('post_'.$postID);
if (!$item->isHit()) {

HEAVY OPERATION <<<<
}

here is problem. i would like to have correct Cache. or in other case it's not so usefull, coz you will have to implement blocking to be sure that code is atomic.

plandem commented Sep 24, 2013

well..this cache is ok for non highload, coz not atomic code.

$item = Yii::$app->cache->getItem('post_'.$postID);
if (!$item->isHit()) {

HEAVY OPERATION <<<<
}

here is problem. i would like to have correct Cache. or in other case it's not so usefull, coz you will have to implement blocking to be sure that code is atomic.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 24, 2013

Member

@plandem

  • getItem doesn't load any data, just instantiating dummy CaceItem.
  • isHit loads the data and returns true if it was loaded.
Member

samdark commented Sep 24, 2013

@plandem

  • getItem doesn't load any data, just instantiating dummy CaceItem.
  • isHit loads the data and returns true if it was loaded.
@plandem

This comment has been minimized.

Show comment
Hide comment
@plandem

plandem Sep 24, 2013

well, there is anyway race condition.

1 thread check isHit and got expired cache, and started to do heavy operation
2 thread same time check isHit and cache is still invalid and started to do heavy operation
3 thread .....
1 thread done heavy operation and refresh cache
2 thread done heavy operation and refresh cache again )and probably data is differ already)
3 thread .....

so problem is still here.

plandem commented Sep 24, 2013

well, there is anyway race condition.

1 thread check isHit and got expired cache, and started to do heavy operation
2 thread same time check isHit and cache is still invalid and started to do heavy operation
3 thread .....
1 thread done heavy operation and refresh cache
2 thread done heavy operation and refresh cache again )and probably data is differ already)
3 thread .....

so problem is still here.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 24, 2013

Member

Sure but you can't simply avoid it w/o backend properly supporting it.

Member

samdark commented Sep 24, 2013

Sure but you can't simply avoid it w/o backend properly supporting it.

@plandem

This comment has been minimized.

Show comment
Hide comment
@plandem

plandem Sep 24, 2013

yep, but i would like to have 'interfaces' at least and ability to implement it transparency if i will need. In other case this cache has quite limited usage.

i mean have 'blocking' mechanism and ability to configure it to use. Without such settings for blocking - just like you wrote, i.e.: yes, we know that there is race condition, if you want to solve it - setup that blocking and our cache will use it to provide atomic code.

plandem commented Sep 24, 2013

yep, but i would like to have 'interfaces' at least and ability to implement it transparency if i will need. In other case this cache has quite limited usage.

i mean have 'blocking' mechanism and ability to configure it to use. Without such settings for blocking - just like you wrote, i.e.: yes, we know that there is race condition, if you want to solve it - setup that blocking and our cache will use it to provide atomic code.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Sep 24, 2013

Member

Well, the PSR basically doesn't cover it at all. So both if we're going to leave it as is or if we're going to change to PSR it's the same in regards to locks.

Member

samdark commented Sep 24, 2013

Well, the PSR basically doesn't cover it at all. So both if we're going to leave it as is or if we're going to change to PSR it's the same in regards to locks.

@plandem

This comment has been minimized.

Show comment
Hide comment
@plandem

plandem Sep 24, 2013

i see :( well...at least i hope you will write about race condition problem at documentation, because a lot of people don't understand and using such 'cache' without any thoughts about it.

Actually, having 'Blocker' at the core of Yii would be nice. As for me it's one of major feature and must be used for any serious site.

plandem commented Sep 24, 2013

i see :( well...at least i hope you will write about race condition problem at documentation, because a lot of people don't understand and using such 'cache' without any thoughts about it.

Actually, having 'Blocker' at the core of Yii would be nice. As for me it's one of major feature and must be used for any serious site.

@Mirocow

This comment has been minimized.

Show comment
Hide comment
@Mirocow

Mirocow commented Sep 26, 2013

+1

@index0h

This comment has been minimized.

Show comment
Hide comment
@index0h

index0h commented Oct 4, 2013

+1

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 17, 2013

Member

PSR is going through another round of discussion so it's better to wait till it is finished. For now we have quite good implementation.

Member

samdark commented Oct 17, 2013

PSR is going through another round of discussion so it's better to wait till it is finished. For now we have quite good implementation.

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Oct 17, 2013

Member

I think we should keep our current implementation at least for 2.0 release. We may consider implementing PSR in 2.1 or later releases.

Member

qiangxue commented Oct 17, 2013

I think we should keep our current implementation at least for 2.0 release. We may consider implementing PSR in 2.1 or later releases.

@samdark

This comment has been minimized.

Show comment
Hide comment
@samdark

samdark Oct 17, 2013

Member

Agree.

Member

samdark commented Oct 17, 2013

Agree.

@samdark samdark closed this Oct 17, 2013

@cebe cebe removed this from the 2.1 milestone Jul 23, 2014

@klimov-paul klimov-paul added this to the 2.1.0 milestone Aug 25, 2017

samdark added a commit that referenced this issue Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment