refactor(search): refactor NUM_RETRIES in esindexbuilder to be configurable #3870
Conversation
|
This pull request is to make the NUM_RETRIES constant in the ESIndexBuilder configurable as an environment variable. The original number is set to 3 which causes the reindex failure when our team build the project. This is because our datahub has too many metadata to reindex and the NUM_RETRIES=3 is not enough for the project to be reindexed. In this case, we think other users in the community may face the same issue as well. So we decide to make this configurable so that the user can change the number as needed. Here attach the log of reindex failure from our team for your reference. |
There was a problem hiding this comment.
Thanks for the contribution. So for env variables, we never change the dockerfiles directly because the variables are not easy to find. Instead we use factories that get variables from application.yaml.
Check out this factory https://github.com/linkedin/datahub/blob/master/metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.java that creates ESIndexBuilders for all our search services. You just need to expose the num retries as an input variable to the constructor and add a new injected variable in the factory similar to this https://github.com/linkedin/datahub/blob/master/metadata-service/factories/src/main/java/com/linkedin/gms/factory/search/ElasticSearchIndexBuilderFactory.java
Then, you can change the application.yaml file that the factory is picking up https://github.com/linkedin/datahub/blob/master/metadata-service/factories/src/main/resources/application.yml#L100 You can see that env variables with defaults are pushed into the application.yml file (and thus into the factory)
|
Hi @dexter-mh-lee I have changed the way of refactor in the way you describe. feel free to add more comments if you have other concerns |
| @@ -100,6 +100,7 @@ elasticsearch: | |||
| prefix: ${INDEX_PREFIX:} | |||
| numShards: ${ELASTICSEARCH_NUM_SHARDS_PER_INDEX:1} | |||
| numReplicas: ${ELASTICSEARCH_NUM_REPLICAS_PER_INDEX:1} | |||
| numRetries: ${ELASTICSEARCH_NUM_RETRIES_PER_INDEX:3} | |||
There was a problem hiding this comment.
nit: Can we rename this to ELASTICSEARCH_INDEX_BUILDER_NUM_RETRIES ?
|
@dexter-mh-lee Thanks for your comment, the variable is renamed. |
Checklist