Add ability to declare Solr object with default commit policy #221

efagerberg · 2017-07-17T19:56:09Z

Since solr has hooks to auto commit after X documents or X seconds, it is best that the Solr methods get, delete, and _update do not have a policy of committing by default.

One may already assume this to be false by default since it is good practice to limit commits. But incase users rely on this functionality, there is the ability to declare the default policy in the Solr constructor.

efagerberg · 2017-07-17T20:46:36Z

@acdha you mind giving this a look over?

acdha

I'm mixed on having the separate _get_commit method - it looks like we could just let _update do that check.

One other thing we should cover in the test case is having tests with commit=None/True/False using Mock to confirm the expected commit value is in the constructed URL.

acdha · 2017-07-19T13:23:37Z

.gitignore

@@ -1,4 +1,6 @@
 .tox
 solr*.tgz
 solr-app
-solr


This change is unrelated

acdha · 2017-07-19T13:27:07Z

pysolr.py

@@ -898,11 +905,12 @@ def add(self, docs, boost=None, fieldUpdates=None, commit=True, softCommit=False
        m = force_unicode(m)

        end_time = time.time()
+        commit = self._get_commit(commit)


Do we really need to make this call here when it's just calling _update() which does the same check? I'm thinking we could remove it in both _add & _delete and inline the _get_commit check into _update.

efagerberg · 2017-07-19T16:46:47Z

Thanks I will review this sometime tomorrow.

acdha · 2017-07-19T17:06:27Z

Thanks for sending the request - it's a good idea but this has been a busy period at work for me to get around to OSS reviews.

efagerberg · 2017-07-19T18:26:51Z

@acdha you were totally right about _get_commit. Fixed that up and added a those tests.

acdha · 2018-01-09T16:09:46Z

Can you rebase this against the current master branch & confirm that the tests still pass?

mlissner · 2018-01-09T16:49:25Z

README.rst

+
+    # Setup a Solr instance. The trailing slash is optional.
+    # All request to solr will result in a commit
+    solr = pysolr.Solr('http://localhost:8983/solr/core_0/', search_handler='/autocomplete', commit_by_default=True)


Maybe this should be something like send_commits or something? by_default doesn't sound right to me.

efagerberg · 2018-02-20T04:07:53Z

@acdha sorry about the wait on this, rebased onto master. @mlissner I have changed the parameter name to always_commit is that more appropriate?

stale · 2018-06-05T14:15:50Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mlissner · 2018-06-05T16:01:06Z

And this should not be closed either. Can't say I'm a fan of the stale bot.

stale · 2018-09-03T16:30:44Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Received all feedback as of PR#151 comment: <#151 (comment)> In particular, better idiomatic python code for the multiple id cases, and an improved test that will delete all documents except one, rather than deleting all in one go.

Rename commit_by_default to always_commit

efagerberg · 2018-09-03T20:19:56Z

@acdha and @mlissner I have updated this branch to master, I hope this still has a chance to be considered as a contribution to this project.

acdha · 2018-09-05T13:31:24Z

Thanks for updating it. It looks good to me. The only question I had is whether we should default to always_commit=True for backwards-compatibility on the assumption that people who aren't pressed on are less likely to read the documentation but I don't feel especially strongly about it.

mlissner · 2018-09-05T16:39:08Z

I think we concluded that switching to always_commit=False was better because it:

would be a good thing for most people, who don't realize that commits should be infrequent in Solr.
would be a good reason to do a major version bump
should have minimal impact because of Solr's default commit policy (which regularly does commits)

mlissner · 2018-09-05T16:39:40Z

Oh, and I do feel pretty strongly about this, because always_commit=True is a weird default that'd definitely mess some people up.

acdha · 2018-09-05T17:26:35Z

Fine by me

efagerberg · 2018-09-06T13:03:58Z

Thanks folks 😄 When this goes out we probably want to make a note very explicitly in the changelog that the default commit policy is to not commit and if user's are upgrading to this version, they need to ensure that if they want to always commit that they change their constructor calls.

acdha · 2018-09-06T14:13:40Z

@efagerberg if you want to draft that wording in a pull request it's a good idea.

efagerberg mentioned this pull request Jul 17, 2017

add() should not commit by default #46

Closed

acdha requested changes Jul 19, 2017

View reviewed changes

acdha previously approved these changes Jan 9, 2018

View reviewed changes

mlissner reviewed Jan 9, 2018

View reviewed changes

stale bot added the stale label Jun 5, 2018

stale bot removed the stale label Jun 5, 2018

stale bot added the stale label Sep 3, 2018

efagerberg dismissed acdha’s stale review via cf12c1c September 3, 2018 20:07

stale bot removed the stale label Sep 3, 2018

Cosimo Streppone and others added 7 commits September 3, 2018 16:16

delete(): improved multiple-id code and tests

5b55731

Received all feedback as of PR#151 comment: <#151 (comment)> In particular, better idiomatic python code for the multiple id cases, and an improved test that will delete all documents except one, rather than deleting all in one go.

Adding more to .gitignore

40dfaf8

Add ability to specify default commit pollicy of Solr object

966aaba

Applying some notes from the PR

f280776

Update PR with master

bbe0e8d

Rename commit_by_default to always_commit

Bringing back deleted new line

d138ec5

Merge branch 'master' into commitByDefault

f8080ec

acdha approved these changes Sep 5, 2018

View reviewed changes

acdha merged commit d0d482b into django-haystack:master Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to declare Solr object with default commit policy #221

Add ability to declare Solr object with default commit policy #221

efagerberg commented Jul 17, 2017 •

edited

efagerberg commented Jul 17, 2017

acdha left a comment

acdha Jul 19, 2017

acdha Jul 19, 2017

efagerberg commented Jul 19, 2017

acdha commented Jul 19, 2017

efagerberg commented Jul 19, 2017

acdha commented Jan 9, 2018

mlissner Jan 9, 2018

efagerberg commented Feb 20, 2018

stale bot commented Jun 5, 2018

mlissner commented Jun 5, 2018

stale bot commented Sep 3, 2018

efagerberg commented Sep 3, 2018

acdha commented Sep 5, 2018

mlissner commented Sep 5, 2018

mlissner commented Sep 5, 2018

acdha commented Sep 5, 2018

efagerberg commented Sep 6, 2018 •

edited

acdha commented Sep 6, 2018

Add ability to declare Solr object with default commit policy #221

Add ability to declare Solr object with default commit policy #221

Conversation

efagerberg commented Jul 17, 2017 • edited

efagerberg commented Jul 17, 2017

acdha left a comment

Choose a reason for hiding this comment

acdha Jul 19, 2017

Choose a reason for hiding this comment

acdha Jul 19, 2017

Choose a reason for hiding this comment

efagerberg commented Jul 19, 2017

acdha commented Jul 19, 2017

efagerberg commented Jul 19, 2017

acdha commented Jan 9, 2018

mlissner Jan 9, 2018

Choose a reason for hiding this comment

efagerberg commented Feb 20, 2018

stale bot commented Jun 5, 2018

mlissner commented Jun 5, 2018

stale bot commented Sep 3, 2018

efagerberg commented Sep 3, 2018

acdha commented Sep 5, 2018

mlissner commented Sep 5, 2018

mlissner commented Sep 5, 2018

acdha commented Sep 5, 2018

efagerberg commented Sep 6, 2018 • edited

acdha commented Sep 6, 2018

efagerberg commented Jul 17, 2017 •

edited

efagerberg commented Sep 6, 2018 •

edited