feat: add neptune proxy #204

friendtocephalopods · 2020-09-24T18:58:58Z

Summary of Changes

add a bunch of test only proxy endpoints to support roundtrip tests.
Use Stat instead of Statistics from latest common.
pin specific common version to reduce surprises (and update to latest common).

Tests

Added abstract, gremlin, neptune tests.
Modified some tests to use Stat instead of Statistics (noop change)

Documentation

Added documentation for how to run roundtrip tests, assuming proper database configuration exists.

CheckList

Make sure you have checked all steps below to ensure a timely review.

PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.
PR includes a summary of changes.
PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
PR passes make test

metadata_service/proxy/atlas_proxy.py

tests/unit/proxy/abstract_proxy_tests.py

alran · 2020-09-25T17:19:12Z

Do any of these files need to be updated / removed now that we have upstreamed with amundsen-gremlin (and since we have changed a lot since Jan)? https://github.com/amundsen-io/amundsenmetadatalibrary/pull/96/files

friendtocephalopods · 2020-09-25T21:58:03Z

Do any of these files need to be updated / removed now that we have upstreamed with amundsen-gremlin (and since we have changed a lot since Jan)? https://github.com/amundsen-io/amundsenmetadatalibrary/pull/96/files

Good question! I think we can rip out some of the aws4auth stuff.

edit: No, I guess it's used by the janus graph tests. Oh well!
edit 2: On further reflection, that makes 0 sense. Removing the dependency here.

feng-tao · 2020-09-28T23:38:12Z

will take a look this week

tests/unit/proxy/abstract_proxy_tests.py

metadata_service/proxy/gremlin_proxy.py

feng-tao

I look at in brief, but it is hard for me to review without much context :( I have a few high level questions:

do you think we could a doc to describe the gremlin in a bit more detail?
How do we ingest metadata for gremlin as I recall square doesn't use databuilder?
Are all the put methods in proxy solely for testing purposes?
what are the features supported for gremlin so far(dataset, user, dashboard with what metadata)?
any todos or issues we keep an eye and how we could extend databuilder to build metadata for neptune?
+1 on @alran 's comment, any cleanup for previous neptune/gremlin proxy?

metadata_service/proxy/base_proxy.py

metadata_service/proxy/gremlin_proxy.py

requirements.txt

feng-tao · 2020-09-30T16:20:54Z

metadata_service/config.py

@@ -77,7 +80,8 @@ class Config:
    PROGRAMMATIC_DESCRIPTIONS_EXCLUDE_FILTERS = []  # type: list


-class LocalConfig(Config):
+# NB: If you're using the gremlin proxy, the appropriate GremlinConfig must be added to any other configs
+class LocalConfig(LocalGremlinConfig, Config):


is subclass GremlinConfig for testing purpose? If that's the case, could we do sth like https://github.com/amundsen-io/amundsenfrontendlibrary/blob/ccfd2d6b82957fef347e956b243e4048c191fc0d/amundsen_application/config.py#L136 which create a test config in the file?

Ah, no, it inherits GremlinConfig so we don't need to litter the metadata service and databuilder config files with our gremlin endpoint information. The alternative is duplicating our gremlin-specific config in 3 different repos, which is unpleasant.

metadata_service/proxy/atlas_proxy.py

feng-tao · 2020-09-30T16:41:06Z

also @AndrewCiambrone if you could take a look and review based on your experience with your gremlin pOC, that will be great!

friendtocephalopods · 2020-09-30T21:07:02Z

do you think we could a doc to describe the gremlin in a bit more detail?

How do we ingest metadata for gremlin as I recall square doesn't use databuilder?

Are all the put methods in proxy solely for testing purposes?

what are the features supported for gremlin so far(dataset, user, dashboard with what metadata)?

any todos or issues we keep an eye and how we could extend databuilder to build metadata for neptune?

+1 on @alran 's comment, any cleanup for previous neptune/gremlin proxy?

Thanks for the review, @feng-tao!

I added some docs in the amundsengremlin readme. The AWS configuration is maybe the most interesting bit of that still pending, but the example code should help folks get started if they knew how to configure AWS and wanted to try the proxy. Is there some additional information you'd like documented here?

We basically have databuilder as an app with cron jobs that fetches data from various sources and then pipes it into the bulk loader using the amundsen-gremlin.

Yep, the put methods in the proxy exist just to test the read code in the proxy along with the write code in amundsengremlin and make sure they agree. Moving them into a separate test class was a good idea, thanks!

Our proxy supports tables, users, but not currently dashboards.

I think one could pretty easily build a publisher for databuilder that uses the amundsengremlin features to lob data into AWS instead of neo4j, we just have a different architecture so we didn't have one built yet! The example code should make it pretty easy if anyone wants to try =)

setup.py

feng-tao · 2020-10-01T17:32:42Z

also, could you rebase with master? There are some small conflict files.

add a bunch of test only proxy endpoints to support roundtrip tests. pin specific common version to reduce surprises. Signed-off-by: Joshua Hoskins <[email protected]>

Per comment, remove deprecated aws4auth code. Signed-off-by: Joshua Hoskins <[email protected]>

Remove typing from requirements Move put methods to a roundtrip test class. Fix super() so this works in roundtrip tests with multiple inheritance. Signed-off-by: Joshua Hoskins <[email protected]>

feng-tao · 2020-10-01T18:16:37Z

thanks, I will do a quick sanity test with our staging with your pr and get back to you.

feng-tao

thanks @friendtocephalopods and everyone who contributed the gremlin integration! lgtm with one nit. And I also do a quick sanity test with the change with our staging as well.

feng-tao · 2020-10-01T18:55:06Z

requirements.txt

 # A common package that holds the models deifnition and schemas that are used
 # accross different amundsen repositories.
-amundsen-common>=0.3.6,<1.0
+amundsen-common==0.5.3


should we still make it a range?

Hmm, sure, we could make it >=0.5.3. This will need to be bumped anytime someone wants to use an updated common object (changes in 0.5.3 meant I had to change this in my PR from 0.5.2!), but we maybe don't care about all minor changes.

>=

feng-tao · 2020-10-01T18:55:13Z

requirements.txt

 # A common package that holds the models deifnition and schemas that are used
 # accross different amundsen repositories.
-amundsen-common>=0.3.6,<1.0
+amundsen-common==0.5.3
+amundsen-gremlin==0.0.4


feng-tao · 2020-10-01T18:56:25Z

tests/unit/proxy/roundtrip/roundtrip_gremlin_proxy.py

+LOGGER = logging.getLogger(__name__)
+
+
+class RoundtripGremlinProxy(GenericGremlinProxy, RoundtripBaseProxy):


thanks, we could file a ticket to ask for help and see if the community could help to implement the round trip test for neo4j/atlas.

Sounds great! I'd definitely suggest taking the approach we did with Neptune -- move the neo4j publishing code from databuilder into a shared lib so we can use the same code both in databuilder to load data in production and in metadata service to validate read/write in the roundtrip tests.

Signed-off-by: Joshua Hoskins <[email protected]>

feng-tao · 2020-10-01T19:11:50Z

thanks, merge it now, may worth sharing in the slack channel for the 3rd option for metadata backend!

friendtocephalopods force-pushed the abstract-proxy-tests branch 5 times, most recently from 139f798 to edea983 Compare September 25, 2020 16:54

friendtocephalopods marked this pull request as ready for review September 25, 2020 16:57

friendtocephalopods requested review from allisonsuarez, bolkedebruin, dikshathakur3119, feng-tao, jinhyukchang, mgorsk1, verdan and a team as code owners September 25, 2020 16:57

alran reviewed Sep 25, 2020

View reviewed changes

metadata_service/proxy/atlas_proxy.py Show resolved Hide resolved

alran reviewed Sep 25, 2020

View reviewed changes

metadata_service/proxy/atlas_proxy.py Outdated Show resolved Hide resolved

alran reviewed Sep 25, 2020

View reviewed changes

tests/unit/proxy/abstract_proxy_tests.py Outdated Show resolved Hide resolved

friendtocephalopods force-pushed the abstract-proxy-tests branch from edea983 to c239a1f Compare September 25, 2020 17:13

friendtocephalopods force-pushed the abstract-proxy-tests branch 2 times, most recently from e4a8ddb to 31a2605 Compare September 28, 2020 20:11

alran reviewed Sep 29, 2020

View reviewed changes

tests/unit/proxy/abstract_proxy_tests.py Outdated Show resolved Hide resolved

alran reviewed Sep 29, 2020

View reviewed changes

tests/unit/proxy/abstract_proxy_tests.py Outdated Show resolved Hide resolved

alran reviewed Sep 29, 2020

View reviewed changes

metadata_service/proxy/gremlin_proxy.py Outdated Show resolved Hide resolved

alran reviewed Sep 29, 2020

View reviewed changes

metadata_service/proxy/gremlin_proxy.py Outdated Show resolved Hide resolved

feng-tao reviewed Sep 29, 2020

View reviewed changes

metadata_service/proxy/base_proxy.py Outdated Show resolved Hide resolved

metadata_service/proxy/gremlin_proxy.py Outdated Show resolved Hide resolved

requirements.txt Outdated Show resolved Hide resolved

feng-tao reviewed Sep 30, 2020

View reviewed changes

metadata_service/proxy/atlas_proxy.py Outdated Show resolved Hide resolved

feng-tao reviewed Oct 1, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

friendtocephalopods force-pushed the abstract-proxy-tests branch from 6d6c7ab to 0f58963 Compare October 1, 2020 16:27

friendtocephalopods added 2 commits October 1, 2020 10:58

metadataservice: add neptune proxy

d882445

add a bunch of test only proxy endpoints to support roundtrip tests. pin specific common version to reduce surprises. Signed-off-by: Joshua Hoskins <[email protected]>

Support roundtrip test segmentation by proxy.

d61bd6b

Per comment, remove deprecated aws4auth code. Signed-off-by: Joshua Hoskins <[email protected]>

friendtocephalopods force-pushed the abstract-proxy-tests branch from 0f58963 to 00f37fa Compare October 1, 2020 18:00

feedback 2

a0e9014

Remove typing from requirements Move put methods to a roundtrip test class. Fix super() so this works in roundtrip tests with multiple inheritance. Signed-off-by: Joshua Hoskins <[email protected]>

friendtocephalopods force-pushed the abstract-proxy-tests branch from 00f37fa to a0e9014 Compare October 1, 2020 18:09

feng-tao approved these changes Oct 1, 2020

View reviewed changes

feedback 3

d4e9fc0

Signed-off-by: Joshua Hoskins <[email protected]>

feng-tao merged commit 09845d4 into amundsen-io:master Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add neptune proxy #204

feat: add neptune proxy #204

friendtocephalopods commented Sep 24, 2020 •

edited

Loading

alran commented Sep 25, 2020

friendtocephalopods commented Sep 25, 2020 •

edited

Loading

feng-tao commented Sep 28, 2020

feng-tao left a comment •

edited

Loading

feng-tao Sep 30, 2020

friendtocephalopods Sep 30, 2020

feng-tao commented Sep 30, 2020

friendtocephalopods commented Sep 30, 2020

feng-tao commented Oct 1, 2020

feng-tao commented Oct 1, 2020

feng-tao left a comment

feng-tao Oct 1, 2020

friendtocephalopods Oct 1, 2020 •

edited

Loading

feng-tao Oct 1, 2020

feng-tao Oct 1, 2020

friendtocephalopods Oct 1, 2020

feng-tao commented Oct 1, 2020

		LOGGER = logging.getLogger(__name__)


		class RoundtripGremlinProxy(GenericGremlinProxy, RoundtripBaseProxy):

feat: add neptune proxy #204

feat: add neptune proxy #204

Conversation

friendtocephalopods commented Sep 24, 2020 • edited Loading

Summary of Changes

Tests

Documentation

CheckList

alran commented Sep 25, 2020

friendtocephalopods commented Sep 25, 2020 • edited Loading

feng-tao commented Sep 28, 2020

feng-tao left a comment • edited Loading

Choose a reason for hiding this comment

feng-tao Sep 30, 2020

Choose a reason for hiding this comment

friendtocephalopods Sep 30, 2020

Choose a reason for hiding this comment

feng-tao commented Sep 30, 2020

friendtocephalopods commented Sep 30, 2020

feng-tao commented Oct 1, 2020

feng-tao commented Oct 1, 2020

feng-tao left a comment

Choose a reason for hiding this comment

feng-tao Oct 1, 2020

Choose a reason for hiding this comment

friendtocephalopods Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

feng-tao Oct 1, 2020

Choose a reason for hiding this comment

feng-tao Oct 1, 2020

Choose a reason for hiding this comment

friendtocephalopods Oct 1, 2020

Choose a reason for hiding this comment

feng-tao commented Oct 1, 2020

friendtocephalopods commented Sep 24, 2020 •

edited

Loading

friendtocephalopods commented Sep 25, 2020 •

edited

Loading

feng-tao left a comment •

edited

Loading

friendtocephalopods Oct 1, 2020 •

edited

Loading