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: Atlas Search Extractor #415

Merged
merged 1 commit into from
Dec 20, 2020

Conversation

mgorsk1
Copy link
Contributor

@mgorsk1 mgorsk1 commented Dec 1, 2020

Summary of Changes

This MR introduces extractor to populate Elasticsearch with Atlas data. This way users can use Atlas as metadata proxy while search using ES Proxy.

Tests

todo

Documentation

todo

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"
  • 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

@mgorsk1 mgorsk1 force-pushed the feature/atlas_search_extractor branch 2 times, most recently from 9c22f99 to 0604f8f Compare December 1, 2020 15:17
@feng-tao
Copy link
Member

feng-tao commented Dec 1, 2020

cc @verdan

@mgorsk1 mgorsk1 changed the title feat: WIP: Atlas Search Extractor feat: Atlas Search Extractor Dec 3, 2020
@mgorsk1 mgorsk1 requested a review from verdan December 7, 2020 12:39
@mgorsk1 mgorsk1 assigned verdan and unassigned mgorsk1 and verdan Dec 7, 2020
@mgorsk1 mgorsk1 force-pushed the feature/atlas_search_extractor branch 2 times, most recently from e7e3552 to 63e60c4 Compare December 9, 2020 13:55
Copy link
Member

@verdan verdan left a comment

Choose a reason for hiding this comment

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

👍 LGTM. only a few nits.

}

def init(self, conf: ConfigTree) -> None:
self.conf = conf.with_fallback(AtlasSearchDataExtractor.DEFAULT_CONFIG)
Copy link
Member

Choose a reason for hiding this comment

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

AtlasSearchDataExtractor.DEFAULT_CONFIG -> self.DEFAULT_CONFIG ?


@property
def entity_type(self) -> str:
return self.conf.get(AtlasSearchDataExtractor.ENTITY_TYPE_KEY)
Copy link
Member

Choose a reason for hiding this comment

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

AtlasSearchDataExtractor.ENTITY_TYPE_KEY -> self.ENTITY_TYPE_KEY

return list(filter(None, input_list))

def _get_driver(self) -> Any:
return Atlas(host=self.conf.get_string(AtlasSearchDataExtractor.ATLAS_URL_CONFIG_KEY),
Copy link
Member

Choose a reason for hiding this comment

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

same with this. self.*

Copy link
Contributor Author

@mgorsk1 mgorsk1 Dec 10, 2020

Choose a reason for hiding this comment

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

I wouldn't change it since it uses proper convention for refering class properties. These are not class instance properties and this approach is used consistently throughout all objects (extractors/publishers)

Copy link
Member

Choose a reason for hiding this comment

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

But you are calling this for this instance only. and not under a static method. 🤷‍♂️

Copy link
Contributor Author

@mgorsk1 mgorsk1 Dec 10, 2020

Choose a reason for hiding this comment

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

Not exactly, please refer to sample_atlas_search_extractor.py to see how those are (or could be) used. Any of those _KEY properties can be used as part of config process for Databuilder Job. It's a general convention used in databuilder afaik and I wouldn't want to make our implementations different.


LOGGER.info(f'Received count: {count}')

if count > 0:
Copy link
Member

Choose a reason for hiding this comment

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

if count:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure !

@mgorsk1
Copy link
Contributor Author

mgorsk1 commented Dec 15, 2020

@feng-tao ready to merge - is it ok to bump version to 4.0.4 ?

@feng-tao
Copy link
Member

feng-tao commented Dec 15, 2020

@mgorsk1 sure! Just remember to list all the commits between 4.0.3 and 4.0.4 when you do the release. thanks.

@feng-tao
Copy link
Member

@mgorsk1 could you fix the lint? I would like to push a new version soon.

Signed-off-by: mgorsk1 <gorskimariusz13@gmail.com>
@mgorsk1 mgorsk1 force-pushed the feature/atlas_search_extractor branch from c5569f9 to ea4887a Compare December 19, 2020 07:32
@feng-tao
Copy link
Member

thanks @mgorsk1 !

@feng-tao feng-tao merged commit 8c63307 into amundsen-io:master Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants