Remove etcd #7
Remove etcd #7
Conversation
…atch rest of KillrVideo
There was a problem hiding this comment.
Hi,
so there are my notices, some of them very minor, like env var name for kafka, and two are serious:
- VERSION file is a dangerous way for versioning. Git tag is more reliable.
- Extraction of docker-compose to a separate repo leads to a more complicated and not-obvious setup.
VERSION
Outdated
| @@ -1 +1 @@ | |||
| DOCKER_BUILD_TAG=0.9.1 | |||
| DOCKER_BUILD_TAG=3.0.1-rc1 | |||
There was a problem hiding this comment.
I can't fully agree with this VERSION file approach. Imho, git tag is much more reliable and obvious source of truth. As long as you have both, you will have inconsistencies between them. Take a look at this:
- We take git tag as a source of truth: https://github.com/KillrVideo/killrvideo-dse-config/blob/master/.travis.yml#L15
- We take hardcoded data in a file as a source of truth https://github.com/KillrVideo/killrvideo-dse-config/blob/1a31a70883597d98b4f90a86986f782d5e3319b4/build/docker-build.sh#L5
It's just a question of time when someone (like me) will forget to update the version file and - ooops - we have a wrong image.
| @@ -8,7 +9,7 @@ class CommentsPublisher(object): | |||
| """Provides methods that publish events associated with the Comments Service.""" | |||
|
|
|||
| def __init__(self): | |||
| self.producer = KafkaProducer(bootstrap_servers='10.0.75.1:9092', | |||
| self.producer = KafkaProducer(bootstrap_servers=os.getenv('KILLRVIDEO_KAFKA', 'kafka'), | |||
There was a problem hiding this comment.
Maybe, KILLRVIDEO_KAFKA_HOST or anything like that? KILLRVIDEO_KAFKA doesn't sound obvious enough.
| @@ -9,7 +10,7 @@ class RatingPublisher(object): | |||
| """Provides methods that publish events associated with the Ratings Service.""" | |||
|
|
|||
| def __init__(self): | |||
| self.producer = KafkaProducer(bootstrap_servers='10.0.75.1:9092', | |||
| self.producer = KafkaProducer(bootstrap_servers=os.getenv('KILLRVIDEO_KAFKA', 'kafka'), | |||
There was a problem hiding this comment.
Same as above, KILLRVIDEO_KAFKA_HOST looks better imho.
| class SuggestedVideoKafkaConsumer(threading.Thread): | ||
| def __init__(self, suggested_videos_consumer): | ||
| threading.Thread.__init__(self) | ||
| self.suggested_videos_consumer = suggested_videos_consumer | ||
|
|
||
| def run(self): | ||
| consumer = KafkaConsumer(bootstrap_servers=BOOTSTRAP_SERVERS, | ||
| consumer = KafkaConsumer(bootstrap_servers=os.getenv('KILLRVIDEO_KAFKA', 'kafka'), |
There was a problem hiding this comment.
Same as above, KILLRVIDEO_KAFKA_HOST looks better imho.
| @@ -9,7 +10,7 @@ class UserManagementPublisher(object): | |||
| """Provides methods that publish events associated with the User Management Service.""" | |||
|
|
|||
| def __init__(self): | |||
| self.producer = KafkaProducer(bootstrap_servers='10.0.75.1:9092', | |||
| self.producer = KafkaProducer(bootstrap_servers=os.getenv('KILLRVIDEO_KAFKA', 'kafka'), | |||
There was a problem hiding this comment.
Same as above, KILLRVIDEO_KAFKA_HOST looks better imho.
| @@ -9,7 +10,7 @@ class VideoCatalogPublisher(object): | |||
| """Provides methods that publish events associated with the Video Catalog Service.""" | |||
|
|
|||
| def __init__(self): | |||
| self.producer = KafkaProducer(bootstrap_servers='10.0.75.1:9092', | |||
| self.producer = KafkaProducer(bootstrap_servers=os.getenv('KILLRVIDEO_KAFKA', 'kafka'), | |||
There was a problem hiding this comment.
Same as above, KILLRVIDEO_KAFKA_HOST looks better imho.
README.md
Outdated
| @@ -15,7 +15,7 @@ Install and run: | |||
| * `python3 -m venv venv` | |||
| * `source venv/bin/activate` | |||
| * Run supporting infrastructure using Docker | |||
| * `./setup-docker.sh` | |||
| * `docker-compose up -d` | |||
| * See the instructions in the [killrvideo-docker-common][https://github.com/KillrVideo/killrvideo-docker-common] repository for running the supporting infrastructure | |||
There was a problem hiding this comment.
Link is not rendered by github. I think the markdown is not correct here.
…A -> KILLRVIDEO_KAFKA_BOOTSTRAP_SERVERS)
Removal of dependence on etcd brings killrvideo-python in line with the rest of the KillrVideo application. Suggest testing with latest killrvideo-docker-common.