Freemabd1/rw 15 #112

freemabd1 · 2017-09-08T19:37:12Z

The pull request is for jira story RW-15. I still need to do alot to make this robust but this is the bare minimum to get connected to BigQuery.

danqrodney

Nice work!

I don't have any comments on the UI code. You may want @blrubenstein or @dmohs to give you feedback there.

If you're eager to get this code in, we can definitely address some of my comments later, but wanted to take a first stab.

danqrodney · 2017-09-08T19:39:54Z

api/build.gradle

  compile 'mysql:mysql-connector-java:+'
  compile 'com.google.cloud.sql:mysql-socket-factory:+'
  compile 'com.google.appengine:appengine:+'
+  compile 'com.google.appengine:appengine-api-1.0-sdk:+'


Which classes required this?

So i get this error without that dependency: java.lang.RuntimeException: Google App Engine runtime detected (the environment variable "com.google.appengine.runtime.version" is set), but unable to resolve appengine-sdk classes. For more details see https://github.com/GoogleCloudPlatform/google-cloud-java/blob/master/APPENGINE.md

Interesting. Thanks for the info!

danqrodney · 2017-09-08T19:50:34Z

api/libproject/devstart.rb

+  common.status "Creating service account for API..."
+  iam_account = "[email protected]"
+  common.run_inline %W{
+    gcloud iam service-accounts keys create sa-key.json --iam-account #{iam_account}


So this will use whatever project and account you might already be gcloud configured to (which may not be the right ones.)

We should probably use --project all-of-us-workbench-test here (assuming that's relevant for this command?) and maybe --account .

Either way, it probably makes sense to try to reuse logic from run_with_creds() or get_service_account_creds_file() in here, as it's doing much the same thing.

I guess i'm still a little confused about this and the above comments. Maybe we can discuss over a call?

Definitely. Let's chat on Monday.

danqrodney · 2017-09-08T19:52:44Z

api/libproject/devstart.rb

  common.status "Running database migrations..."
  common.run_inline %W{docker-compose run db-migration}
+  common.status "Creating service account for API..."
+  iam_account = "[email protected]"


I'm thinking we may just want to use [email protected] here, for a couple reasons:

Our server will need to talk to Firecloud, not just BigQuery; we'll be using the creds for that, too.

We already have to configure things so that this service account has access to FireCloud and BigQuery to get our test env working; we can avoid having additional configuration by reusing it for local development.

Note that if you use run_with_creds() / get_service_account_creds_file() -- see below -- it will use that service account.

we can certainly do that. I don't have access to create a service account under @appspot.gserviceaccount.com. I can currently only create under all-of-us-workbench-test.iam.gserviceaccount.com and @all-of-us-ehr-dev.iam.gserviceaccount.com

You don't need to create a service account, it already exists. (It's the SA that our deployed AppEngine app in test uses.)

You will need a key for it, but you are an Owner on all-of-us-workbench-test, so you should have permissions to do that in cloud console or on the command line (like this).

danqrodney · 2017-09-08T19:56:10Z

api/src/main/java/org/pmiops/workbench/api/CohortBuilderController.java

+
+        QueryJobConfiguration queryConfig =
+                QueryJobConfiguration.newBuilder(
+                        "SELECT id, " +


Maybe define constants for our queries?

danqrodney · 2017-09-08T19:57:51Z

api/src/main/java/org/pmiops/workbench/api/CohortBuilderController.java

+                                "is_selectable, " +
+                                "domain_id " +
+                                "FROM `pmi-drc-api-test.synpuf.icd9_criteria` " +
+                                "where parent_id = 0 " +


I assume at some point the "0" here would be a parameter set to parentId, as described here:

https://cloud.google.com/bigquery/docs/parameterized-queries

Is that right?

Right.. that will definitely change.

danqrodney · 2017-09-08T19:58:25Z

api/src/main/java/org/pmiops/workbench/api/CohortBuilderController.java

+                        .setUseLegacySql(false)
+                        .build();
+
+        // Create a job ID so that we can safely retry.


We may want a helper class that does all this boiler plate whenever running a blocking BigQuery query.

Alternatively, maybe we could use the query() method instead of creating a job?

http://googlecloudplatform.github.io/google-cloud-java/0.2.7/apidocs/com/google/cloud/bigquery/BigQuery.html#query(com.google.cloud.bigquery.QueryRequest)

Agreed.. i think the link about also uses query()

danqrodney · 2017-09-08T20:09:34Z

api/src/main/java/org/pmiops/workbench/api/CohortBuilderController.java

+        CriteriaListResponse criteriaResponse = new CriteriaListResponse();
+        for (List<FieldValue> row : result.iterateAll()) {
+            final Criteria criteria = new Criteria();
+            criteria.setId(row.get(0).getLongValue());


I forsee this method of extracting results from queries as a bit brittle / hard to follow, since we're using indexes here instead of column names.

What may make sense: have a helper that runs the query, constructs a map of column name to index using this method:

http://googlecloudplatform.github.io/google-cloud-java/0.2.7/apidocs/com/google/cloud/bigquery/QueryResult.html#schema()

and then lets you retrieve values from rows by column name instead of index (by looking up the index.)

danqrodney · 2017-09-08T20:10:07Z

api/src/main/resources/workbench.yaml

+        description: type of criteria
+        type: string
+      code:
+        description: what level of data access the user has


This looks wrong?

danqrodney · 2017-09-08T20:10:49Z

api/src/main/resources/workbench.yaml

+        description: specifies of child or parent
+        type: boolean
+      selectable:
+        description: specifies if use can search with


Which criteria would we not be able to search on?

danqrodney · 2017-09-08T20:11:38Z

api/src/main/resources/workbench.yaml

+        type: integer
+        format: int64
+      group:
+        description: specifies of child or parent


"child" or "parent"?

Is the idea that nodes which are "parent" are not selectable, and are just containers for children?

some are not selectable and some are depending on the specific criteria tree, so we use a combination of isGroup and IsSelectable to determine what is a parent or leaf and is selectable

So "group" = "true" means there are children, and "selectable" = "true" means you can query on it?

s-rubenstein

All of this from a UI side looks pretty reasonable. I left the office before I got a chance to look at it, so I haven't actually been able to pull it down and play with it.

danqrodney · 2017-09-11T20:16:34Z

api/src/main/java/org/pmiops/workbench/api/CohortBuilderController.java

+
+    private static final Logger log = Logger.getLogger(CohortBuilderController.class.getName());
+
+    public static final String CRTIERIA_QUERY = "SELECT id, type, code, name, est_count, is_group, is_selectable, domain_id\n"


There are some long lines in here. We should wrap them.

danqrodney · 2017-09-11T20:17:57Z

api/src/main/java/org/pmiops/workbench/api/CohortBuilderController.java

+
+    private static final Logger log = Logger.getLogger(CohortBuilderController.class.getName());
+
+    public static final String CRTIERIA_QUERY = "SELECT id, type, code, name, est_count, is_group, is_selectable, domain_id\n"


CRTIERIA -> CRITERIA

danqrodney · 2017-09-11T20:18:31Z

api/src/main/java/org/pmiops/workbench/api/CohortBuilderController.java

+        return response.getResult();
+    }
+
+    protected Map<String, Integer> getResultMapper(QueryResult result) {


Eventually we'll probably want to move this and some of the code above into some BigQuery utils class. (Fine to leave it here for now.)

freemabd1 added 3 commits September 8, 2017 12:22

adding api changes

250efad

adding new wizard components

2becee2

RW-15 cleanup of code for this story

80da71f

freemabd1 assigned danqrodney Sep 8, 2017

danqrodney reviewed Sep 8, 2017

View reviewed changes

s-rubenstein approved these changes Sep 8, 2017

View reviewed changes

freemabd1 added 3 commits September 11, 2017 09:32

RW-15 updated to proper description

f430f82

RW-15 implemented parameterized query

3b0ebfe

RW-15 added a couple of helper methods

fa029e6

danqrodney approved these changes Sep 11, 2017

View reviewed changes

RW-15 reformatted sql statement and corrected a misspelling

91889fc

danqrodney approved these changes Sep 11, 2017

View reviewed changes

freemabd1 merged commit 1e248cf into master Sep 11, 2017

freemabd1 deleted the freemabd1/RW-15 branch September 13, 2017 15:48


		private static final Logger log = Logger.getLogger(CohortBuilderController.class.getName());

		public static final String CRTIERIA_QUERY = "SELECT id, type, code, name, est_count, is_group, is_selectable, domain_id\n"

Freemabd1/rw 15 #112

Freemabd1/rw 15 #112

Uh oh!

Conversation

freemabd1 commented Sep 8, 2017

Uh oh!

danqrodney left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-rubenstein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants