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

Caught exceptions are still logged #52

Closed
tewe opened this issue Mar 24, 2014 · 7 comments
Closed

Caught exceptions are still logged #52

tewe opened this issue Mar 24, 2014 · 7 comments
Labels

Comments

@tewe
Copy link

tewe commented Mar 24, 2014

A client may mark items read multiple times due to bad connectivity.

fever.py line 164 deals with that, but peewee still generates a scary log entry.

I noticed that the code uses exceptions-as-control-flow all over the place. Is this a good idea?

@passiomatic
Copy link
Owner

fever.py line 164 deals with that, but peewee still generates a scary log entry.

Yes, it is a bit annoying to see that exceptions in the log file. Currently I let Peewee to log the exception by setting level to WARN in init.py:

logging.getLogger("peewee").setLevel(logging.WARN)

One idea to change this could be to silence Peewee logger altogether (or raise its level to CRITICAL) unless Coldsweat loglevel is different from DEBUG.

I noticed that the code uses exceptions-as-control-flow all over the place. Is this a good idea?

That code style is - generally speaking - the Python-way to deal with operations that potentially could not succeed. See for example: Why is it “Easier to ask forgiveness than permission” in python, but not in Java?

@tewe
Copy link
Author

tewe commented Mar 24, 2014

I still think asking forgiveness is a bad idea when dealing with something as unforgiving as an RDBMS.

In this case peewee offers .get_or_create rather than .get. But worryingly that doesn't use a transaction, and we'd lose the debug message.

@passiomatic
Copy link
Owner

I've never used .get_or_create since it is deprecated since version 2.0 of Peewee (http://peewee.readthedocs.org/en/latest/peewee/api.html#Model.get_or_create).

Anyway, in the specific example we are discussing about a corner case. You are right when you write we are "...dealing with something unforgiving as an RDBMS" and that's exactly why I let RDBMS to decide if it can accept or not the new Read record. The RDBMS is more rigid than the outside world when it comes to enforce integrity.

@tewe
Copy link
Author

tewe commented Mar 25, 2014

The problem with letting the database decide is that, once it decides you made an error, you've left normalcy. SQL logs might fill up. Shared hosting might freeze you. People get paged out of bed.

And peewee does nothing to adjust that to what might be pythonic, it also treats an error as an error, and logs it.

So your code has the two layers it depends on hyperventilating all the time because it's too lazy to do basic sanity checks.

Sorry if that came across strong, in this case it's a really small issue.

passiomatic added a commit that referenced this issue Mar 25, 2014
@passiomatic
Copy link
Owner

Just to recap, here there's where I'm trapping IntegrityError in the code:

fetcher.py

  • add_subscription, to check if user has already a feed in her subscriptions

fever.py

  • mark_command, to check if entry/feed/group/all has been marked as read
  • mark_command, to check if an entry has been marked as saved

frontend.py

  • _mark_entry, mark an entry as read/saved
  • entry_list_mark, to check if an entry has been marked as read

models.py

  • setup, to bootstrap database model schema - this happens once per user

In fever and frontend modules I already take extra steps to ensure a myriad of IntegrityError exceptions aren't raised when marking every thing as read, so the issue should be very limited.

As anticipated I've made the Peewee and Requests loggers less verbose while in production (level != DEBUG). This should alleviate the pain :)

Let me know if it works for you.

@passiomatic
Copy link
Owner

Ah, now understands why Peewee started out of the blue to log errors that previously ignored: coleifer/peewee#254

Looks like other people are experiencing this issue. I'll keep an eye on it.

@passiomatic
Copy link
Owner

For the record: next Peewee release won't log errors — coleifer/peewee@69db852

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

No branches or pull requests

2 participants