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

Log errors and proceed when reindexing #172

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tobiasherp
Copy link

We have a ZODB which contains at least one object (after processing of 13000 objects) which can't be reindexed.

Of course we do want the remaining 50000 objects to be reindexed, and we'd like to know which one causes the trouble.

Thus,

  • we log the error, and
  • we count the errors.

ZODB ConflictErrors are not handled that way (yet?) but re-raised.

(... which have been created by virtualenv .)
We have a ZODB which contains at least one object (after processing of
13000 objects) which can't be reindexed.

Of course we do want the remaining 50000 objects to be reindexed,
and we'd like to know *which one* causes the trouble.

Thus,
- we log the error, and
- we count the errors.

ZODB ConflictErrors are not handled that way (yet?) but re-raised.
- Added ImportError to bare except statement
- removed unused variable e
@tobiasherp
Copy link
Author

PyLint complains about an unused variable, but it is wrong: That e variable is used, in fact.
It is contained in the locals() result which is given to the logger.error call.

This is a little bit "special", but there is a good reason I use this syntax all the time:
If, according to the current log level, the log message doesn't need to be written, it is not formatted, either.

So, what do we do about this?

tobiasherp and others added 3 commits July 26, 2019 17:08
In my Plone 4.3 setup, I got an AttributeError rather than an KeyError.
With this change, the reindexing worked, and the single broken
CatalogBrain was logged while the remaining 59644 have been reindexed.
@tobiasherp
Copy link
Author

tobiasherp commented Jul 26, 2019

Sorry for the noise.
The most important change seems to be to catch AttributeErrors as well in the utils.get_valid_objects function.

try:
# this exception must be resubmitted:
from ZODB.POSException import ConflictError
except ImportError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't require it, better to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to add ZODB to setup.py

catalog.catalog_object(obj, idxs=['object_provides'], update_metadata=False)
except ConflictError:
raise
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea; if you don't know what other exception can be raised here, better to remove this also or we can hide some other issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this operation be better atomic? The only improvement that could be made is to log the object that failed but raise the exception right away. If the upgrade runs without error, many users may think that everything went well when it didn't.

@idgserpro
Copy link
Member

idgserpro commented Jul 30, 2019

@tobiasherp does the upgrade step not stop when a ConflictErrors occurs but stop for when another error occurs? It is?

try:
# this exception must be resubmitted:
from ZODB.POSException import ConflictError
except ImportError:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best to add ZODB to setup.py

catalog.catalog_object(obj, idxs=['object_provides'], update_metadata=False)
except ConflictError:
raise
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this operation be better atomic? The only improvement that could be made is to log the object that failed but raise the exception right away. If the upgrade runs without error, many users may think that everything went well when it didn't.

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

Successfully merging this pull request may close these issues.

3 participants