Adding resource_organization_id, person_organization_id, piperson_organization_id to cloudfact tables #1956
Conversation
…anization_id columns to cloudfact tables
eiffel777
requested review from
jpwhite4,
ryanrath,
aaronweeden,
connersaeli and
aestoltm
There was a problem hiding this comment.
Does there need to be a PR into xdmod-xsede that updates these files to add the piperson_organization_id column?
configuration/etl/etl_action_defs.d/cloud_access/access_cloud_metrics_aggregation.json
configuration/etl/etl_action_defs.d/cloud_access/access_cloud_metrics_aggregation_by_day.json
configuration/etl/etl_tables.d/cloud_jetstream/jetstream_cloudfact_by_.json
configuration/etl/etl_tables.d/cloud_jetstream/jetstream_cloudfact_by_day.json
| @@ -27,8 +27,10 @@ | |||
| "year": "${:YEAR_VALUE}", | |||
| "${AGGREGATION_UNIT}": "${:PERIOD_VALUE}", | |||
| "host_resource_id": "sr.resource_id", | |||
| "resource_organization_id": "sr.resource_organization_id", | |||
There was a problem hiding this comment.
It looks like the Cloud realm uses service_provider instead of resource_organization_id — why is resource_organization_id needed?
There was a problem hiding this comment.
I removed this. It should probably change to using resource_organization_id in the future since there is a common group by for service provider that uses that column
configuration/etl/etl_action_defs.d/cloud_common/session_records.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
The comment on line 6 says Euca — should that be Cloud?
There was a problem hiding this comment.
It probably should but I will change that some other time in a different PR. I want to keep this PR focused on just the organization ids.
configuration/etl/etl_action_defs.d/cloud_common/cloud_metrics_aggregation.json
Show resolved
Hide resolved
configuration/etl/etl_tables.d/cloud_common/cloudfact_by_day.json
Outdated
Show resolved
Hide resolved
…umns and remove resource_organization_id column from cloud realm
Co-authored-by: Aaron Weeden <31246768+aaronweeden@users.noreply.github.com>
@aaronweeden This was added in https://github.com/ubccr/xdmod-xsede/pull/530 |
There was a problem hiding this comment.
When comparing this file with xdmod-xsede: configuration/etl/etl_action_defs.d/cloud_access/access_cloud_metrics_aggregation.json, it looks like to compute num_sessions_ended, this file uses start_event_type_id while the xdmod-xsede file uses end_event_type_id.
There was a problem hiding this comment.
I think this is best addressed in a separate PR.
| @@ -29,6 +29,7 @@ | |||
| "host_resource_id": "sr.resource_id", | |||
| "account_id": "sr.account_id", | |||
There was a problem hiding this comment.
Does this need to be COALESCE(sr.account_id, -1)? That's how it is in xdmod-xsede: configuration/etl/etl_action_defs.d/cloud_access/access_cloud_metrics_aggregation.json. It looks like there are some other fields like person_id that are going from a nullable column to a non-nullable column so there might need to be some more COALESCEs.
There was a problem hiding this comment.
There probably are other fields COALESCEs should be put on, but I think is better put in a new PR since that affects fields not related to this PR.
configuration/etl/etl_action_defs.d/cloud_common/session_records.json
Outdated
Show resolved
Hide resolved
configuration/etl/etl_action_defs.d/cloud_common/session_records.json
Outdated
Show resolved
Hide resolved
configuration/etl/etl_action_defs.d/cloud_common/session_records.json
Outdated
Show resolved
Hide resolved
Co-authored-by: Aaron Weeden <31246768+aaronweeden@users.noreply.github.com>
Description
This adds the Resource, PI, and Person organization ID columns for the cloud realm. These columns are used in the xsede module at this point in time.
Motivation and Context
We want to track the organizations that a person, resource and PI belong to.
Tests performed
Tested in docker
Checklist: