feat: Implement configurable minimum number of readers for popular tables #146

jacobhjkim · 2020-07-05T14:08:52Z

Summary of Changes

Related issue: #534

This PR adds POPULAR_TABLE_MINIMUM_READER_COUNT to the config file and num_readers variable for the neo4j popular table query. This allows the minimum number of readers for popular tables to be configurable.

Tests

N/A

Documentation

No documentation changes at this time

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

codecov-commenter · 2020-07-05T14:10:37Z

Codecov Report

Merging #146 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   72.01%   72.06%   +0.04%     
==========================================
  Files          26       26              
  Lines        1197     1199       +2     
  Branches      121      121              
==========================================
+ Hits          862      864       +2     
  Misses        308      308              
  Partials       27       27

Impacted Files	Coverage Δ
metadata_service/config.py	`100.00% <100.00%> (ø)`
metadata_service/proxy/neo4j_proxy.py	`75.00% <100.00%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f4cb7c...20c52b8. Read the comment docs.

…efault_args

…ADER_COUNT as config variable

jornh · 2020-07-08T11:07:46Z

Thanks for picking it up @jacobhjkim LGTM 👍

Need Tao or someone else from Lyft to merge it.

mgorsk1 · 2020-07-08T12:09:50Z

I am adding FrequentUsers functionality for AtlasProxy (PR 147) now and was thinking on something similar, will wait with my merge until this one is in 🤞

feng-tao · 2020-07-09T03:59:45Z

sorry for the delay in review, will take a look!

feng-tao

lgtm except one 1 nit

metadata_service/config.py

feng-tao

lgtm, thanks @jacobhjkim !

jacobhjkim · 2020-07-09T05:45:47Z

metadata_service/config.py

@@ -64,6 +64,9 @@ class Config:
    # On User detail method, these keys will be added into amundsen_common.models.user.User.other_key_values
    USER_OTHER_KEYS = {'mode_user_id'}  # type: Set[str]

+    # Number of minimum reader count to qualify for popular table
+    POPULAR_TABLE_MINIMUM_READER_COUNT = 10  # type: int


POPULAR_TABLE_MINIMUM_READER_COUNT = os.environ.get('POPULAR_TABLE_MINIMUM_READER_COUNT', 10) # type: int

It should have been this. 🙃

not sure, but we could override it through different config right during the actual deployment? For example, at Lyft we override the config with production configuration with our internal deployment code.

Right, production deployment should have some other config class. False alarm

* commit '369685cc715e95af82dfa4dc14d0c58af8bb1ac9': chore: replace references to Lyft -> Amundsen (amundsen-io#174) feat: Data Owner Implementation of Atlas Proxy (amundsen-io#156) chore: fix docker push action (amundsen-io#172) chore: add docker publish action and remove travis (amundsen-io#171) chore: add pypi publish action (amundsen-io#170) fix: removing OidcConfig file and making statsd configurable through envrionment variable (amundsen-io#157) ci: add dependabot config (amundsen-io#169) Update repo name in travis file (amundsen-io#163) feat: Populate is_view property in AtlasProxy (amundsen-io#155) fix: Overlapping table name issue in Readers [AtlasProxy] feat: Add resource_reports field in Table API ( Atlas proxy) (amundsen-io#149) chore: apply license headers to all the source files (amundsen-io#153) feat: Add programmatic descriptions parser for [AtlasProxy] (amundsen-io#152) feat: Add Frequent Users feature in [AtlasProxy] (amundsen-io#147) feat: Implement configurable minimum number of readers for popular tables (amundsen-io#146) chore: update the email for the project (amundsen-io#148) # Conflicts: # README.md # docs/configurations.md # docs/structure.md # metadata_service/config.py # metadata_service/oidc_config.py # metadata_service/proxy/neo4j_proxy.py # requirements.txt # setup.py

Jacob Kim added 3 commits July 5, 2020 18:10

Add num_readers to get_popular_tables and _get_popular_tables_uris

49c9aaa

Update metadata service to accept num_readers for popular tables

7d3d597

Update unittest for popular tables

3d4127f

jacobhjkim changed the title ~~Implement configurable minimum number of readers for popular tables~~ feat: Implement configurable minimum number of readers for popular tables Jul 5, 2020

Rename popular test unittest to test_should_get_popular_tables_with_d…

5a0cbfe

…efault_args

jacobhjkim mentioned this pull request Jul 5, 2020

fix: Properly request number of entries for popular tables amundsen-io/amundsenfrontendlibrary#502

Merged

5 tasks

Jacob Kim added 3 commits July 6, 2020 13:39

Revert changes to popular_tables API and add POPULAR_TABLE_MINIMUM_RE…

cbd6930

…ADER_COUNT as config variable

pep8

9031bbb

revert swagger doc

175dba7

mgorsk1 mentioned this pull request Jul 8, 2020

feat: Add Frequent Users feature in [AtlasProxy] #147

Merged

5 tasks

feng-tao previously approved these changes Jul 9, 2020

View reviewed changes

metadata_service/config.py Outdated Show resolved Hide resolved

Default POPULAR_TABLE_MINIMUM_READER_COUNT 4 -> 10

20c52b8

jacobhjkim dismissed feng-tao’s stale review via 20c52b8 July 9, 2020 04:04

feng-tao approved these changes Jul 9, 2020

View reviewed changes

feng-tao merged commit 19b5c7b into amundsen-io:master Jul 9, 2020

jacobhjkim deleted the flexible-popular-tables branch July 9, 2020 04:22

jacobhjkim commented Jul 9, 2020

View reviewed changes

jornh mentioned this pull request Aug 4, 2020

Feature Proposal: Make popularity number of users configurable amundsen-io/amundsen#534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement configurable minimum number of readers for popular tables #146

feat: Implement configurable minimum number of readers for popular tables #146

jacobhjkim commented Jul 5, 2020 •

edited

Loading

codecov-commenter commented Jul 5, 2020 •

edited

Loading

jornh commented Jul 8, 2020

mgorsk1 commented Jul 8, 2020

feng-tao commented Jul 9, 2020

feng-tao left a comment

feng-tao left a comment

jacobhjkim Jul 9, 2020

feng-tao Jul 9, 2020 •

edited

Loading

jacobhjkim Jul 9, 2020

feat: Implement configurable minimum number of readers for popular tables #146

feat: Implement configurable minimum number of readers for popular tables #146

Conversation

jacobhjkim commented Jul 5, 2020 • edited Loading

Summary of Changes

Tests

Documentation

CheckList

codecov-commenter commented Jul 5, 2020 • edited Loading

Codecov Report

jornh commented Jul 8, 2020

mgorsk1 commented Jul 8, 2020

feng-tao commented Jul 9, 2020

feng-tao left a comment

Choose a reason for hiding this comment

feng-tao left a comment

Choose a reason for hiding this comment

jacobhjkim Jul 9, 2020

Choose a reason for hiding this comment

feng-tao Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

jacobhjkim Jul 9, 2020

Choose a reason for hiding this comment

jacobhjkim commented Jul 5, 2020 •

edited

Loading

codecov-commenter commented Jul 5, 2020 •

edited

Loading

feng-tao Jul 9, 2020 •

edited

Loading