Skip to content
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

db component usage hardcode #43

Open
ilyachase opened this issue Sep 27, 2019 · 3 comments
Open

db component usage hardcode #43

ilyachase opened this issue Sep 27, 2019 · 3 comments

Comments

@ilyachase
Copy link

ilyachase commented Sep 27, 2019

Hi there.
In file BeanstalkController.php from line 305 to 316 there is a lot of logic with a lot of assumptions.

  1. First of all, why do you assume that any yii2 application has 'db' component? In each getDb() you are trying to get it without any checks. It results in Exception 'yii\base\InvalidConfigException' with message 'Unknown component ID: db'. In my app I don't have db component, but have mongodb.
  2. Second, why do you assume that every yii2 that uses yii2-beanstalk needs to reconnect to db periodically? What if my app doesn't even use db in beanstalk workers? Moreover, I think it's developer's responsibility to make sure that db connection is up and available. If you want to put this logic in your extension, at least give it option to disable. For example, in Laravel this check happens by catching exceptions after any db query and checking if it's caused by has gone away error. If it is, it's just reconnect and executes query again.
  3. Why do you assume that reconnection every hardcoded 60 * 60 seconds will help? This value should correlate with wait_timeout of the db.
  4. Making some hardcoded text query in BeanstalkController.php:140 is a very big assumtion. I don't think you should every do that. You don't know what database the app uses and don't know what it supports. And there is one more 31536000 value without any ability to change or disable it.
@udokmeci
Copy link
Owner

Hi @ilyachase ,

  1. This is most common usage i guess but I left it unprotected so if you like to have something different than overriding setDBSessionTimeout method will do the work.

I assume, assumption for generality is a good thing because people like to have their extension turnkey but also easy to customize.

  1. Reconnectiong to DB is another work everytime you need. Easier to connect once in a while waiting than the error. But if you really into writing the code needed be my guest.

About 3 you are right. We better make it settable.

  1. When you extending your own class you can also override either mysqlSessionTimeout or setDBSessionTimeout.

@ilyachase
Copy link
Author

ilyachase commented Sep 30, 2019

@udokmeci My point is that your extension is hard coupled to Mysql and the 'db' component, which isn't covered in the docs at all.
My use case was that I used your extension and don't have 'db' and even Mysql, and my worker errors every hour.
I think at least three things should be done:

  1. Before accessing 'db', check if it exists in yii.
  2. Make option to disable auto-reconnection without extending.
  3. Cover it in docs.

If you agree, I can help with PR.

@udokmeci
Copy link
Owner

@ilyachase
You are 100% right. I think it is better to start working on it now!
Sorry for late reply.

Regards,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants