Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/977#issuecomment-169101930
The code changes looks OK. I am not an expert on the Solr code, but I do
have a few concerns.
In response to comments from @HeartSaVioR and @dossett. It looks like the
`commitStgy.commit()` is not needed at all. That method appears to return a
true if a commit should happen. We are forcing a commit with the tick tuple so
calling it and checking the response is not needed. Which brings up my
concern. There is no documentation in CommitStrategy about how it is intended
to be used. Which methods are called when, etc. Without them it is hard to
tell if we are violating some type of contract by making this change. For now
I think this is OK, but it would be good to add in that documentation, and I
would love it to see another method added to the commit strategy (perhaps
beyond the scope of this code). That indicates a commit happened. So it would
look something like.
```
/**
* Strategy defining when the Solr Bolt should commit the request batch to
Solr.
*/
public interface SolrCommitStrategy extends Serializable {
/**
* @return true of a commit should happen.
*/
boolean shouldCommit();
/**
* Indicates that a commit occurred. This may happen even if
shouldCommit returned false, or was not even called at all to indicate that a
commit was forced.
*/
void commitHappened();
/**
* Indicates that a tuple was added to the current batch.
*/
void update();
}
```
The current count code has issues because it does not know that the commit
happened, so it uses a modulo with a check for 0, which means we could get
really odd situations where a commit was forced because of a timeout, but the
next batch only has a single entry in it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
