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

Add zopectl.command for reindexing #88

Merged
merged 3 commits into from
Dec 9, 2015

Conversation

tschorr
Copy link
Contributor

@tschorr tschorr commented Oct 30, 2015

This adds a zopectl.command entry point for reindexing Solr. I needed this because when calling @@solr-maintenance/reindex in the browser, reindexing stopped after ~ 7500 documents, with still ~90000 documents to go. I guess it is the browser that eventually closes the connection.
The PR also fixes a problem in _get_site with recent Zope versions (args has changed).

@tisto
Copy link
Member

tisto commented Nov 4, 2015

@tschorr travis build is broken. Would you mind checking? I would merge the reindex_solr script right away. The _get_site fix would need some more explanation and testing I guess. Maybe it would be better to separate those two issues. BTW: Please update the changelog.

@tschorr
Copy link
Contributor Author

tschorr commented Nov 5, 2015

So first, Travis: The build terminated while trying to download the Solr tarball. This should be unrelated to my changes. I would restart the build, but I am not allowed to do so.

@tisto
Copy link
Member

tisto commented Nov 5, 2015

I re-ran the build. It is green but there is an unreported code violation. We need to fix the travis conf...

@tschorr
Copy link
Contributor Author

tschorr commented Nov 5, 2015

As for the _get_site fix, I'm still googling for where and when the change occured that causes the bug in the original _get_site version.
Maybe a likely candidate is
plone/plone.recipe.zope2instance@7c52c42 , but I'm not 100% sure.
It happens in Zope2.Startup.zopectl, starting at line 374 in run_entrypoints/go. Here the comment says "remove -c and add command name as sys.argv[0]". But when I put in a debugger, I can see that sys.argv has already 3 elements and '-c' is the middle one. What gets removed is the last element, resulting in ['-c', 'solr_reindex|clear', 'site_name'] being passed to the entry point.
I'm using Zope2-2.13.22, plone.recipe.zope2instance-4.2.15.

Bottom line, without the fix, none of the entry points will work, and my suggestion would be to leave it in this PR. Also, the fix should be save to use with older versions Zope2/plone.recipe.zope2instance.
I'll google some more and will add a comment in the code when I update the changelog.

+1 for more tests, but in this case I'm not yet sure where to start...

@tschorr
Copy link
Contributor Author

tschorr commented Nov 7, 2015

I finally decided to not rely on positional arguments and rather use a flag to pass the site id. It looks like plone.recipe.zope2instance should override Zope.Startup.zopectl.ZopeCmd's run_entrypoint method. This is also the place where tests should be added.
I have added a comment in _get_site and updated the changelog.

# name as the first argument and any further arguments after that, but that does not
# work with plone.recipe.zope2instance. Using positional arguments therefore
# is unreliable - resolve to using a flag.
parser = argparse.ArgumentParser()
Copy link
Member

Choose a reason for hiding this comment

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

@tschorr if we rely on argparse, don't we have to add it to the dependencies (Python 2.7)?

@tschorr
Copy link
Contributor Author

tschorr commented Dec 8, 2015

argparse is part of the Python standard library since the first Python 2.7 release:
http://svn.python.org/projects/python/tags/r27/Misc/NEWS (you can search for "6247")
So I'd say it's not necessary to add it to the dependencies.

@tisto
Copy link
Member

tisto commented Dec 8, 2015

Ok, I guess we have to decide if we drop Python 2.6 support or add it as a dependency.

@tschorr
Copy link
Contributor Author

tschorr commented Dec 8, 2015

Added, with a comment (for easier removal later).

tisto added a commit that referenced this pull request Dec 9, 2015
Add zopectl.command for reindexing
@tisto tisto merged commit a564725 into collective:master Dec 9, 2015
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.

2 participants