Skip to content
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

feat(neo4j): Allow datahub to connect to specific neo4j database #9179

Conversation

deepgarg-visa
Copy link
Contributor

By default datahub connects to default db when neo4j is being used for Graph service implementation. With this PR, we can provide specific database in neo4j to connect to.
Also, changes default value of graph_service_impl in docker quickstart files

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@github-actions github-actions bot added the devops PR or Issue related to DataHub backend & deployment label Nov 6, 2023
@david-leifker david-leifker self-assigned this Nov 7, 2023
@@ -146,7 +146,7 @@ services:
- ELASTICSEARCH_INDEX_BUILDER_MAPPINGS_REINDEX=true
- ELASTICSEARCH_INDEX_BUILDER_SETTINGS_REINDEX=true
- ELASTICSEARCH_BUILD_INDICES_CLONE_INDICES=false
- GRAPH_SERVICE_IMPL=elasticsearch
- GRAPH_SERVICE_IMPL=neo4j
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets not change the graph service impl here. If you'd like to make it configurable, it can be like GRAPH_SERVICE_IMPL=${GRAPH_SERVICE_IMPL:-elasticsearch} so that you can override the impl with an environment variable if that's your preferred quickstart backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Please check

@@ -146,7 +146,7 @@ services:
- ELASTICSEARCH_INDEX_BUILDER_MAPPINGS_REINDEX=true
- ELASTICSEARCH_INDEX_BUILDER_SETTINGS_REINDEX=true
- ELASTICSEARCH_BUILD_INDICES_CLONE_INDICES=false
- GRAPH_SERVICE_IMPL=elasticsearch
- GRAPH_SERVICE_IMPL=neo4j
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, please make it configurable: GRAPH_SERVICE_IMPL=${GRAPH_SERVICE_IMPL:-elasticsearch}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@david-leifker
Copy link
Collaborator

Thanks! If we can restore the default and allow overriding of the graph impl, then this looks good to me!

… "python generate_docker_quickstart.py check-all"
@deepgarg-visa
Copy link
Contributor Author

@david-leifker
task python generate_docker_quickstart.py check-all was failing, so I made changes in docker-compose.override.yml and run task python3 generate_docker_quickstart.py generate-all on my local, which reorders env variables in files docker-compose.quickstart.yml and docker-compose-m1.quickstart.yml

# Conflicts:
#	docker/quickstart/docker-compose-m1.quickstart.yml
#	docker/quickstart/docker-compose.quickstart.yml
@david-leifker
Copy link
Collaborator

LGTM! Thanks!

@david-leifker david-leifker merged commit 399e032 into datahub-project:master Nov 8, 2023
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops PR or Issue related to DataHub backend & deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants