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

wish: Expose some fields on StatusUpdaterBolt to allow custom naming strategy implementation #591

Closed
jcruzmartini opened this Issue Jul 10, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@jcruzmartini
Contributor

jcruzmartini commented Jul 10, 2018

Hi @jnioche , we are working with ES integration of the crawler and for some reasons we want to extend the functionality of StatusUpdaterBolt , basically we want to apply our custom naming strategy for handling many status indexes and content indexes names in parallel. Names will depend on same information stored previously in metadata. eg. status_${some_property_name}.
The existing code fits perfect to our needs, so we want to extend existing class and only override behavior that you have on store method.

Our main issue basically consist in the limited scope of all variables in StatusUpdaterBolt that we need to interact in store method of our child class that will be extending StatusUpdaterBolt.

Having those vars as protected or having getters for those will allow to extend this class and allow custom implementations in a future.

Obviously, we can implement our whole custom implementation extending AbstractStatusUpdaterBolt, but just want to check if you want to avoid overriding of this class for any special reason.

Thanks for your time in advance.

@jcruzmartini jcruzmartini changed the title from question / wish : Expose some fields on StatusUpdaterBolt to allow custom naming strategy implementation to Expose some fields on StatusUpdaterBolt to allow custom naming strategy implementation Jul 10, 2018

@jcruzmartini jcruzmartini changed the title from Expose some fields on StatusUpdaterBolt to allow custom naming strategy implementation to wish: Expose some fields on StatusUpdaterBolt to allow custom naming strategy implementation Jul 10, 2018

@jnioche

This comment has been minimized.

Member

jnioche commented Jul 11, 2018

Hi
@jcruzmartini I suppose your only modification to the logic in store() would be to use a custom index name based on the value of a metadatum. Instead of copying the whole method and have access to the fields, what about adding a method getIndexName(Metadata m) in the super class which would return the value set in the config. Your subclass would simply need to override that method and could use super.getIndexName(m).

What do you think? I can see a benefit of doing this for cases where we want to keep a separate index per language for instance.

I am not against setting the fields to protected but I am not convinced that it is necessary for all of them. Also, I wouldn't want the subclass to be able to change the values and would prefer getters.

@xytian315 would the solution above work for you as well?

Would either of you like to submit a PR?

Thanks

@jnioche jnioche added this to the 1.11 milestone Jul 11, 2018

@jcruzmartini

This comment has been minimized.

Contributor

jcruzmartini commented Jul 11, 2018

Thanks @jnioche for your quick response, your proposal sounds great. I am gonna be creating a pull request for including those changes. I am thinking that probably we will need to do same in AbstractSpout class for getting custom indexName when executing ES query in AggregationSpout.
I am gonna talk with @xytian315 in order to coordinate modifications required for both changes.

@jnioche

This comment has been minimized.

Member

jnioche commented Jul 11, 2018

Except that it can't be based on Metadata values in the spouts. I'd create a constructor for the AbstractSpout where you can specify a non-default index name, similar to what I did for the IndexerBolt

jcruzmartini added a commit to jcruzmartini/storm-crawler that referenced this issue Jul 12, 2018

jcruzmartini added a commit to jcruzmartini/storm-crawler that referenced this issue Jul 12, 2018

jcruzmartini added a commit to jcruzmartini/storm-crawler that referenced this issue Jul 12, 2018

jcruzmartini added a commit to jcruzmartini/storm-crawler that referenced this issue Jul 12, 2018

jcruzmartini added a commit to jcruzmartini/storm-crawler that referenced this issue Jul 12, 2018

@jnioche

This comment has been minimized.

Member

jnioche commented Jul 13, 2018

See #592

@jnioche jnioche closed this Jul 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment