Skip to content

Add configs for the search service - #2182

Merged
eugene-sy merged 20 commits into
feature/search-servicefrom
agni/tabernacle-configs
Jul 3, 2017
Merged

Add configs for the search service#2182
eugene-sy merged 20 commits into
feature/search-servicefrom
agni/tabernacle-configs

Conversation

@kairome

Copy link
Copy Markdown
Contributor

No description provided.

@kjanoszkjanosz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Preliminary review, please ask someone from devops team to have a look at it.

EDIT:
I've forgotten that there is no devops handle on GH 😺

Comment threadtabernacle/ansible/group_vars/all Outdated
phoenix_tax_rule_rate: 7.5

# Agni
agni_src: ../../search-service

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we rename search-service dir to agni then?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think yes, but we can do it in next PR, when you finish the draft of DSL (to avoid conflicts).

},
"env": {
"PORT": "{{agni_port}}",
"SEARCH_SERVER": ""

@kjanoszkjanoszJun 14, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That should not be empty afaik, as it points to elastic search instance which is crucial for search-service/agni to work

{% include "core-backend/isaac.json.j2" %},
{% include "core-backend/solomon.json.j2" %},
{% include "core-backend/middlewarehouse.json.j2" %}
{% include "core-backend/agni.json.j2" %}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: spacing

@kairomekairome added the WIP label Jun 14, 2017
@kairome
kairomeforce-pushed the agni/tabernacle-configs branch from 63b2e8c to 11c2d22CompareJune 14, 2017 15:17

@eugene-syeugene-sy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general it looks good.
Except comments in the code, could you add ability to change service port in the resource file?

},
"ports": [{{agni_port}}],
"healthChecks": [{
"path": "/ping",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AFAIK we don't have /ping endpoint right now, health check will be failed and it will break deployment.
I think it's a good idea to change it to consumer/greenriver health check (by process) or implement the endpoint

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep, I think it's better to implement the endpoint

@kjanoszkjanoszJun 16, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've added it in this PR

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yep, saw that. Tested already, thanks!

@kairome
kairomeforce-pushed the agni/tabernacle-configs branch 2 times, most recently from feb45a6 to 010ea20CompareJune 16, 2017 10:26
@kairome
kairomeforce-pushed the agni/tabernacle-configs branch 4 times, most recently from f1aa2d4 to 2241db3CompareJune 23, 2017 12:10
@kairome
kairomeforce-pushed the agni/tabernacle-configs branch from 2241db3 to e7b3418CompareJune 23, 2017 12:56
@kairome
kairome changed the base branch from feature/search-service to apple-pay-frontendJune 30, 2017 10:47
@kairome
kairome changed the base branch from apple-pay-frontend to feature/search-serviceJune 30, 2017 10:47
"maximumOverCapacity": 0
},
"env": {
"PORT": "{{agni_port}}",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe it can work on a random port. Why do we need to specify it explicitly?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

You know, I'm not sure why 🙂
I'll change it to a random port

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe all marathon apps should use port from env PORT to be compatible with them.

"SEARCH_SERVER": "elasticsearch://{{search_server}}",
"JAVA_OPTS":"-XX:+UseConcMarkSweepGC -Xms512m -Xmx512m"
},
"ports": [{{agni_port}}],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line not necessary too.

@eugene-sy
eugene-sy merged commit 3ab111e into feature/search-serviceJul 3, 2017
@eugene-sy
eugene-sy deleted the agni/tabernacle-configs branch July 3, 2017 17:48
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kairome@narma@eugene-sy@kjanosz