GA360 Importing into Snowflake #715
Conversation
pwnage101
force-pushed
the
pwnage101/snowflake
branch
14 times, most recently
from
0009464 to
3a66285
Compare
pwnage101
force-pushed
the
pwnage101/snowflake
branch
4 times, most recently
from
dbc02ba to
d2497bf
Compare
pwnage101
force-pushed
the
pwnage101/snowflake
branch
2 times, most recently
from
c1b5ba7 to
0ea353b
Compare
| """ | ||
| The null sequence in the data to be copied. Default is Hive NULL (\\N). | ||
| """ | ||
| return r'\\N' |
There was a problem hiding this comment.
reviewers: I just moved these two methods into an abstract subclass.
| @@ -75,6 +75,7 @@ edx.analytics.tasks = | |||
| run-vertica-sql-scripts = edx.analytics.tasks.warehouse.run_vertica_sql_scripts:RunVerticaSqlScriptTask | |||
| test-vertica-sqoop = edx.analytics.tasks.common.vertica_export:VerticaSchemaToBigQueryTask | |||
| load-ga-permissions = edx.analytics.tasks.warehouse.load_ga_permissions:LoadGoogleAnalyticsPermissionsWorkflow | |||
| ga-imports = edx.analytics.tasks.warehouse.ga_imports:SnowflakeLoadGATask | |||
There was a problem hiding this comment.
I'm not really sure which class to reference here. It seems to work as-is, despite the fact that i'm calling a different task from the command line (SnowflakeLoadGAIntervalTask).
pwnage101
force-pushed
the
pwnage101/snowflake
branch
from
57c5d25 to
bff93e8
Compare
pwnage101
force-pushed
the
pwnage101/snowflake
branch
from
bff93e8 to
25c64d2
Compare
Codecov Report
@@ Coverage Diff @@
## master #715 +/- ##
==========================================
- Coverage 75.18% 74.95% -0.24%
==========================================
Files 203 205 +2
Lines 22888 23073 +185
==========================================
+ Hits 17209 17294 +85
- Misses 5679 5779 +100
Continue to review full report at Codecov.
|
| if not aws_secret_access_key: | ||
| aws_secret_access_key = self._get_s3_config('aws_secret_access_key') | ||
| if 'host' not in kwargs: | ||
| kwargs['host'] = self._get_s3_config('host') or 's3.amazonaws.com' |
There was a problem hiding this comment.
reviewers: All of this logic was removed because:
- The aws access/secret key was duplicate code (this logic already exists inside of the luigi.contrib.s3 library).
- The host argument can be supplied in the luigi .cfg file instead of in code. Besides, once we upgrade to boto3 there will be no need for setting this default.
| @property | ||
| def output_format(self): | ||
| """ | ||
| The type of compression to use for the output files. |
There was a problem hiding this comment.
Did you mean the output format here ?
There was a problem hiding this comment.
It's the output file format, or the compression format for the output file, same thing right?
There was a problem hiding this comment.
I just found it a little ambiguous, as both output_compression_type and output_format methods have the same docstrings. One is the exported file format and the other is the compression type to use for exported files. Just thought we could use different docstrings.
There was a problem hiding this comment.
Oh, whoops, fixing
|
Per a conversation with @brianhw I'm going to split this up into two branches: one including the updates that don't require a newer Luigi, and one including updates that do require a newer Luigi. |
|
Closing in favor of https://github.com/edx/edx-analytics-pipeline/pull/721 |
DE-1374
analytics-secure PR: https://github.com/edx-ops/analytics-secure/pull/234
Analytics Pipeline Pull Request
Make sure that the following steps are done before merging: