[RW-2704][risk=low] Split up listCluster API and move initialization logic to the client-side. #3135

gjuggler · 2020-02-14T22:18:05Z

This PR resolves RW-2704 by splitting up the server-side listCluster endpoint into component get/create/delete pieces.

As a result of that server-side simplification, the initialization state management and retry logic had to move somewhere. I opted to implement this on the client side, since we already had a few client-side classes which were effectively repeating the same or similar initialization logic (notebook-redirect, interactive-notebook, and reset-cluster-button).

The resulting controller class, cluster-initializer, is meant to consolidate this logic into one well-tested module. Each React component uses this in a more-or-less simple way (see below for notes on why reset-cluster-button is special) with the Promise based API.

Some notes:

Unit testing Promises with time-based delays isn't currently possible in Jest (see useFakeTimers breaks with native promise implementation jestjs/jest#7151). I had originally implemented the polling loop using a while loop within an async function, but I ultimately switched to using a poll() method and setTimeout to make unit testing more feasible.
The reset cluster button is a bit of a special case here, since we don't want the button to take any action until the user explicitly chooses to "Reset" (this is the whole point of RW-2704). The solution I took was to create a "no-action" initializer when the button is first loaded, and to create a "action-ready" initializer when the button is clicked. It's a bit awkward, but less awkward than almost all other options I could think of.

Ultimately, while working through this PR it became clear that this functionality should really live somewhere on the server-side, ideally within the analysis application (e.g. Leo) itself. That would allow Terra and AoU, for example, to simplify both of our clients. Something worth thinking about as Framework platform convergence discussions continue.

calbach · 2020-02-14T22:55:01Z

api/src/main/resources/workbench.yaml

+        200:
+          description: Returns the cluster that was created for this user and workspace.
+          schema:
+            $ref: '#/definitions/CreateClusterResponse'


Ideal REST semantics would have getCluster and createCluster return a Cluster directly rather than wrapping

calbach · 2020-02-14T22:56:02Z

ui/src/app/utils/cluster-initializer.tsx

+
+export class ClusterInitializer {
+  private workspaceNamespace: string;
+  private onStatusUpdate: (ClusterStatus) => void;


calbach · 2020-02-14T23:06:08Z

ui/src/app/utils/cluster-initializer.tsx

+      if (this.isClusterErrored()) {
+        // TODO: report errors to Stackdriver here.
+        console.log('Cluster error -- deleting for retry');
+        await this.deleteCluster();


Per ticket discussion, I might move this above the loop so it only happens on initialization - errors encountered later could always throw and require user intervention. I think this limits the risk of concurrent pollers causing cluster churn - delete is the only really problematic/destructive action here. The rest should be relatively safe for concurrent requests.

Sorry, I owed you a reply on this one. I thought hard about treating delete cluster specially somehow, but ultimately I concluded that it was simpler & more readable, and arguably not too much more dangerous, to have it treated the same as our other state transitions. My hope is that the other guard rails (max initialization time, max delete count, max 500s) will help mitigate any potential out of control loops.

Yes makes sense, I surmised as much from the changes - thanks.

calbach · 2020-02-14T23:09:27Z

ui/src/app/utils/cluster-initializer.tsx

+        if (isAbortError(e)) {
+          return Promise.reject();
+        }
+        // If we received a NOT_FOUND error, we need to create a cluster for this workspace.


Probably a pre-emptive comment, but this should actually check for NOT_FOUND

calbach · 2020-02-14T23:15:34Z

ui/src/app/utils/cluster-initializer.tsx

+    // Overall strategy: continue polling the get-cluster endpoint, with capped exponential backoff,
+    // until we either reach our goal state (a RUNNING cluster) or run up against the overall
+    // timeout threshold.
+    while (!this.isClusterRunning()) {


One thing which isn't covered here yet is error retries. Since this is a very long polling cycle (expected to commonly be ~7-10m), would be ideal if this were resilient to the occasional transient 500. Using this package on the outgoing calls would probably suffice:

workbench/ui/src/app/utils/retry.tsx

Line 8 in d86dead

export async function fetchAbortableRetry<T>(fetchFn: () => Promise<T>, timeoutMillis: number, maxRetries: number): Promise<T> {

(e.g. only fail the polling loop if the request fails X times in a row)

gjuggler · 2020-02-21T15:10:25Z

@calbach this is finally worth another look. I'm really not in love with how complex the solution got, but IMO it's probably better to have one gnarly-but-well-tested thing rather than simpler-looking-but-more-fragile logic spread around in a few components. Curious to hear whether you agree.

@jaycarlton as I warned you, here's a client-side PR that feels like a server-side PR (for obvious reasons...). Feel free to just browse, or treat this like a full code review & dig into questionable-looking areas, etc.

Both: I pushed a relatively-recent version of this branch to my test GAE service, accessible here: https://gjordan-dot-all-of-us-workbench-test.appspot.com/ There's one known issue I need to fix (the cluster reset button looks weird while initially fetching cluster status), but please use that to run it through the ringer.

jaycarlton · 2020-02-21T15:33:37Z

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

-      throw new BadRequestException("Must specify billing project");
-    }
+  public ResponseEntity<Cluster> getCluster(String workspaceNamespace) {
+    String firecloudWorkspaceName = lookupWorkspace(workspaceNamespace).getFirecloudName();


nit: can the body of this function be pushed into the WorkspaceService, and have it return Optional<Cluster>?

IMO it wouldn't be worth it. This controller is <10 lines (including wrapping!) which is way shorter than before, and very much in the spirit of https://github.com/all-of-us/workbench/blob/master/api/doc/code-structure.md#controllers.

(And, upon reading that section again, I would probably disagree that 'controllers should contain no actual logic'. IMO, having some calls to well-factored access enforcement and validity checking service methods is more reasonably the scope of a controller than of a service.)

I can go along with that. I just haven't see much of a clean example of how to draw the line. If we're not really MVC, then I don't know what the "C" means.

jaycarlton

Here's my backend notes.

For the big PRs, I like to pull the branch so I can navigate better. The downside is that I might have seen things that are slightly outside the scope of the PR 🙈 .

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

jaycarlton · 2020-02-21T15:43:17Z

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

+    workspaceService.enforceWorkspaceAccessLevel(
+        workspaceNamespace, firecloudWorkspaceName, WorkspaceAccessLevel.WRITER);
+
+    this.leonardoNotebooksClient.deleteCluster(


nit: drop the this..

Oops. It gets hard switching between Typescript and Java sometimes...

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

api/src/main/resources/workbench.yaml

jaycarlton · 2020-02-21T16:41:19Z

api/src/test/java/org/pmiops/workbench/api/ClusterControllerTest.java

  // a workspace's namespace is always its billing project ID
  private static final String WORKSPACE_NS = BILLING_PROJECT_ID;
-  private static final String WORKSPACE_ID = "wsid";
-  private static final String WORKSPACE_NAME = "wsn";
+  // Workspace ID is also known as firecloud_name. This identifier is generated by


This comment would be very useful in the code as well.

I added it to the workbench.yaml parameters definition description, which feels like the most centralized place where this lives. Maybe it could go on the DB model class as well, but we tend to not have as much comment type stuff over there.

jaycarlton · 2020-02-21T16:42:02Z

api/src/test/java/org/pmiops/workbench/api/ClusterControllerTest.java

-  @Autowired ClusterAuditor clusterAuditor;
-  @Autowired LeonardoNotebooksClient notebookService;
-  @Autowired FireCloudService fireCloudService;
+  @MockBean AdminActionHistoryDao adminActionHistoryDao;


nit: I like to name mocked members with a mock prefix, especially for large test classes.

jaycarlton · 2020-02-21T16:42:49Z

api/src/test/java/org/pmiops/workbench/api/ClusterControllerTest.java

+  @MockBean ComplianceService complianceService;
+  @MockBean DirectoryService directoryService;
+  @MockBean FireCloudService fireCloudService;
+  @MockBean LeonardoNotebooksClient notebookService;


Is this name intentional? Looks like there's a separate org.pmiops.workbench.notebooks.NotebooksService.

jaycarlton · 2020-02-21T16:50:22Z

api/src/test/java/org/pmiops/workbench/api/ClusterControllerTest.java


-    assertThrows(
-        ForbiddenException.class, () -> clusterController.listClusters(WORKSPACE_NS, WORKSPACE_ID));
+    assertThrows(ForbiddenException.class, () -> clusterController.getCluster(WORKSPACE_NS));
    verify(workspaceService, never()).validateActiveBilling(anyString(), anyString());


Do we still need the first doThrow if we're going to verify that the method that does the throwing isn't called? Here and below.

Good catch – fixed.

jaycarlton

All the nits that's fit to crit....ique.

ui/src/app/pages/analysis/interactive-notebook.tsx

ui/src/app/pages/analysis/notebook-redirect.tsx

ui/src/app/pages/analysis/reset-cluster-button.tsx

jaycarlton · 2020-02-21T20:48:29Z

ui/src/app/utils/cluster-initializer.tsx

+    // Fetch the current cluster status, with some graceful error handling for NOT_FOUND response
+    // and abort signals.
+    try {
+      this.currentCluster = await this.getCluster();


Why not have getCluster() call onStatusUpdate() itself?

I think it's easier to follow when all of the status updates are happening in the main poll method.

jaycarlton · 2020-02-21T20:49:41Z

ui/src/app/utils/cluster-initializer.tsx

+          new ClusterInitializationFailedError('Abort signal received during cluster API call',
+            this.currentCluster));
+      } else if (this.isNotFoundError(e)) {
+        // A not-found error is somewhat expected, if a cluster has recently been deleted or


Ideally Leo would expose info on recently deleted clusters (say for 7 days) so that we don't need to guess.

It does expose that information via getClusters?includeDeleted=true. However, whether or not it was deleted is irrelevant here. In either situation we're going to create a new cluster.

jaycarlton · 2020-02-21T20:50:28Z

ui/src/app/utils/cluster-initializer.tsx

+      } else if (this.isClusterErrored()) {
+        // If cluster is in error state, delete it so it can be re-created at the next poll loop.
+        reportError(
+          `Cluster ${this.currentCluster.clusterNamespace}/${this.currentCluster.clusterName}` +


nit: exposing a property for the full cluster path/fqn would be cleaner than doing it here I think.

jaycarlton · 2020-02-21T20:51:18Z

ui/src/app/utils/cluster-initializer.tsx

+      }
+    }
+
+    setTimeout(() => this.poll(), this.currentDelay);


nit: do we already have backoff delay programmed someplace? Seems odd for it to live here.

We have some stuff over here http://github.com/all-of-us/workbench/blob/67ab8fb211b1b76440cff340343c7a107903a87b/ui/src/app/utils/retry.tsx#L69-L69 but nothing which fit this use case. Trying to generalize this would have caused more maintenance overhead than inlining the two lines to increment exponential decay.

OK, but there's also a lot of maintenance overhead if we have multiple, slightly inconsistent ways of doing basic things like retrying.

jaycarlton · 2020-02-21T20:52:10Z

ui/src/app/utils/errors.tsx

@@ -29,7 +29,7 @@ export function reportError(err: Error) {

 /** Returns true if the given error is an AbortError, as used in fetch() aborts. */
 export function isAbortError(e: Error) {
-  return (e instanceof DOMException) && e.name === 'AbortError';
+  return e instanceof DOMException && e.name === 'AbortError';


nit: fwiw I think the parentheses help here.

calbach

Thanks! Client side looks reasonable, mostly small things. Thanks for the detailed comments.

calbach · 2020-02-21T19:30:27Z

ui/src/app/pages/analysis/interactive-notebook.tsx

-          retry();
-        });
+    private async runCluster(onClusterReady: Function): Promise<void> {
+      const initializer = new ClusterInitializer({


nit: looking at this interface, I'm wondering whether it is worth exposing this class at all. Seems like a static helper like initializeCluster({ ... }); might restrict the API surface a bit more clearly, since the current API is basically: create instance, call run() exactly once.

Sure, I can take a stab at that. I ended up modeling this as a static method on ClusterInitializer rather than a separately-exported function. LMK if you feel it looks out of place and we should really only export a wrapper function.

calbach · 2020-02-21T19:34:42Z

ui/src/app/pages/analysis/reset-cluster-button.tsx

-
-  private pollClusterTimer: NodeJS.Timer;
+  private aborter = new AbortController();
+  private initializer?: ClusterInitializer;


Why do we need to hold this reference?

calbach · 2020-02-21T19:41:10Z

ui/src/app/pages/analysis/reset-cluster-button.tsx

+      await this.initializer.run();
+      this.setState({isPollingCluster: false});
+    } catch (e) {
+      if (this.aborter.signal.aborted) {


opt: this may be a more direct way of checking:

workbench/ui/src/app/utils/errors.tsx

Line 31 in f1d264a

export function isAbortError(e: Error) {

I think I'd originally had that, but the awkward thing here is that the ClusterInitializer captures its own AbortError and throws a more generic initialization failure error (

workbench/ui/src/app/utils/cluster-initializer.tsx

Line 246 in 67ab8fb

if (isAbortError(e)) {

).

I'll attempt to clarify this by doubling-down on the error subclassing approach, with a more specific "abort error" thrown from the initializer.

calbach · 2020-02-22T00:19:50Z

ui/src/app/pages/analysis/reset-cluster-button.tsx

+      // If the initializer has completed and the status is null, it means that
+      // a cluster doesn't exist for this workspace.
+      return this.createTooltip(
+        'You have not yet created a notebook server for this workspace.',


I think this language is slightly misleading, I would suggest:

"You do not currently have an active notebook server for this workspace."

Good point – done. (I assume this is mostly because we leave no clusters behind when we run the periodic reset-everything cron.)

calbach · 2020-02-22T00:24:05Z

ui/src/app/utils/cluster-initializer.spec.tsx

+    p.then(() => isSettled = true).catch((e) => {
+      isSettled = true;
+      // Re-throw the error.
+      // throw e;


calbach · 2020-02-22T00:56:35Z

ui/src/app/utils/cluster-initializer.tsx

+export class ClusterInitializationFailedError extends Error {
+  public readonly cluster: Cluster;
+
+  constructor(message: string, cluster?: Cluster) {


In Typescript you can just do constructor(private readonly cluster?: Cluster) and remove the rest of the instance property boilerplate

calbach · 2020-02-22T01:08:02Z

ui/src/app/utils/cluster-initializer.tsx

+ *    App Engine server-side request timeout. To reliably implement this control on the server side
+ *    would likely require new database persistence, tasks queues, and additional APIs in order to
+ *    provide the client with status updates.
+ *  - Ultimately, we might expect this type of functionality ("Get me a cluster for workspace X and


I'm not sure I agree with this statement. We've conceptually merged several Leo concepts into one within the AoU product, primarily on account of product decisions to avoid exposing the inner workings of the cluster state. This creates some impedance mismatches which generates complexity here. For example, we don't really expose the fact that a cluster can become paused, or that it can become deleted. If we decided to surface these things (as Terra UI probably does), it would create less business logic here.

Polling cannot be removed, unless cluster creation is redesigned and made instantaneous somehow. Even if we pushed the maximum amount of logic into Leo, the operation is fundamentally asynchronous.

Logic around when to create/start/stop a cluster at the request of a user needs to live at the product level.

"recreate this cluster" could potentially be a Leo operation, but since we customize Leo clusters anyways, we probably want a hand in recreating clusters anyways.

The other avenue we could explore would be to say that Leo should not have an error state, but ultimately some cluster creations will fail regardless of how many retries Leo adds. So I do think a product-level cluster error state is still a necessity.

Good points all around — but I still feel that if Leo had an async API which was effectively "Create or reuse a cluster with config X, for project Y and user Z, and get it to a running state", it would be heavily used by both Terra and AoU, since to both of these UIs it's effectively a means to an end (which is to have a Jupyter notebook running in my browser).

Yeah, the less we know about the underlying clusters the better. We shouldn't know whether we have a new or existing cluster ideally (provided we can supply config).

calbach · 2020-02-22T01:10:35Z

ui/src/app/utils/cluster-initializer.tsx

+const DEFAULT_MAX_CREATE_COUNT = 2;
+const DEFAULT_MAX_DELETE_COUNT = 2;
+const DEFAULT_MAX_RESUME_COUNT = 2;
+const DEFAULT_MAX_SERVER_ERROR_COUNT = 5;


5 seems reasonable for consecutive errors, but might be a bit low for global error count during the polling cycle. May want to make this distinction.

I'm wary of adding too much more complexity without letting this settle in for awhile. Okay if I bump to 10 and call it a day?

calbach · 2020-02-22T01:12:09Z

ui/src/app/utils/cluster-initializer.tsx

+    if (this.abortSignal && this.abortSignal.aborted) {
+      // We'll bail out early if an abort signal was triggered while waiting for the poll cycle.
+      return this.reject(
+        new ClusterInitializationFailedError('Request was aborted.', this.currentCluster));


opt nit: probably wouldn't put trailing punctuation in my error messages, though I don't know we have any real conventions/guidelines on this.

Ack – done.

ui/src/app/utils/cluster-initializer.tsx

gjuggler

Thanks for the reviews!

gjuggler · 2020-02-24T15:10:47Z

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

-      throw new BadRequestException("Must specify billing project");
-    }
+  public ResponseEntity<Cluster> getCluster(String workspaceNamespace) {
+    String firecloudWorkspaceName = lookupWorkspace(workspaceNamespace).getFirecloudName();


IMO it wouldn't be worth it. This controller is <10 lines (including wrapping!) which is way shorter than before, and very much in the spirit of https://github.com/all-of-us/workbench/blob/master/api/doc/code-structure.md#controllers.

(And, upon reading that section again, I would probably disagree that 'controllers should contain no actual logic'. IMO, having some calls to well-factored access enforcement and validity checking service methods is more reasonably the scope of a controller than of a service.)

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

gjuggler · 2020-02-24T15:12:20Z

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

+    workspaceService.enforceWorkspaceAccessLevel(
+        workspaceNamespace, firecloudWorkspaceName, WorkspaceAccessLevel.WRITER);
+
+    this.leonardoNotebooksClient.deleteCluster(


Oops. It gets hard switching between Typescript and Java sometimes...

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

gjuggler · 2020-02-24T15:14:53Z

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


-    FirecloudWorkspace fcWorkspace;
+    FirecloudWorkspace firecloudWorkspace;


gjuggler · 2020-02-24T22:09:45Z

ui/src/app/utils/cluster-initializer.tsx

+    if (this.abortSignal && this.abortSignal.aborted) {
+      // We'll bail out early if an abort signal was triggered while waiting for the poll cycle.
+      return this.reject(
+        new ClusterInitializationFailedError('Request was aborted.', this.currentCluster));


Ack – done.

ui/src/app/utils/cluster-initializer.tsx

gjuggler · 2020-02-24T22:14:30Z

ui/src/app/utils/cluster-initializer.tsx

+    // Fetch the current cluster status, with some graceful error handling for NOT_FOUND response
+    // and abort signals.
+    try {
+      this.currentCluster = await this.getCluster();


I think it's easier to follow when all of the status updates are happening in the main poll method.

gjuggler · 2020-02-24T22:16:03Z

ui/src/app/utils/cluster-initializer.tsx

+      if (this.isClusterErrored()) {
+        // TODO: report errors to Stackdriver here.
+        console.log('Cluster error -- deleting for retry');
+        await this.deleteCluster();


Sorry, I owed you a reply on this one. I thought hard about treating delete cluster specially somehow, but ultimately I concluded that it was simpler & more readable, and arguably not too much more dangerous, to have it treated the same as our other state transitions. My hope is that the other guard rails (max initialization time, max delete count, max 500s) will help mitigate any potential out of control loops.

gjuggler · 2020-02-24T22:18:10Z

ui/src/app/utils/cluster-initializer.tsx

+      }
+    }
+
+    setTimeout(() => this.poll(), this.currentDelay);


We have some stuff over here http://github.com/all-of-us/workbench/blob/67ab8fb211b1b76440cff340343c7a107903a87b/ui/src/app/utils/retry.tsx#L69-L69 but nothing which fit this use case. Trying to generalize this would have caused more maintenance overhead than inlining the two lines to increment exponential decay.

gjuggler · 2020-02-25T18:53:39Z

Should be ready for another look now

jaycarlton · 2020-02-26T15:40:06Z

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

-      throw new BadRequestException("Must specify billing project");
-    }
+  public ResponseEntity<Cluster> getCluster(String workspaceNamespace) {
+    String firecloudWorkspaceName = lookupWorkspace(workspaceNamespace).getFirecloudName();


I can go along with that. I just haven't see much of a clean example of how to draw the line. If we're not really MVC, then I don't know what the "C" means.

jaycarlton · 2020-02-26T15:43:57Z

api/src/main/java/org/pmiops/workbench/workspaces/WorkspaceServiceImpl.java

-    fcResponse = fireCloudService.getWorkspace(workspaceNamespace, workspaceId);
+    fcResponse =
+        fireCloudService.getWorkspace(
+            dbWorkspace.getWorkspaceNamespace(), dbWorkspace.getFirecloudName());


I was thinking @ericsong was leading the charge to get these out of the service level. Maybe I misunderstood.

api/src/main/resources/workbench.yaml

ui/src/app/pages/analysis/interactive-notebook.tsx

ui/src/app/pages/analysis/notebook-redirect.tsx

ui/src/app/utils/cluster-initializer.spec.tsx

jaycarlton · 2020-02-26T16:11:53Z

ui/src/app/utils/cluster-initializer.tsx

+ *    App Engine server-side request timeout. To reliably implement this control on the server side
+ *    would likely require new database persistence, tasks queues, and additional APIs in order to
+ *    provide the client with status updates.
+ *  - Ultimately, we might expect this type of functionality ("Get me a cluster for workspace X and


Yeah, the less we know about the underlying clusters the better. We shouldn't know whether we have a new or existing cluster ideally (provided we can supply config).

ui/src/app/utils/cluster-initializer.tsx

jaycarlton · 2020-02-26T16:18:15Z

ui/src/app/utils/cluster-initializer.tsx

+      }
+    }
+
+    setTimeout(() => this.poll(), this.currentDelay);


OK, but there's also a lot of maintenance overhead if we have multiple, slightly inconsistent ways of doing basic things like retrying.

calbach

Sorry for the delay. Looks good, thanks!

calbach · 2020-02-26T18:44:07Z

api/src/main/java/org/pmiops/workbench/notebooks/LeonardoNotebooksClientImpl.java

@@ -1,5 +1,7 @@
 package org.pmiops.workbench.notebooks;

+import static org.pmiops.workbench.exceptions.ExceptionUtils.convertNotebookException;


nit: Ignore if this has already been discussed to death elsewhere, but not a big fan of this static import. In general I don't love them outside of tests and in this case it's not saving much space below

Hah – I'm a blind slave to IntelliJ, and they put the "import static..." option above the "quality usage of..." so I chose the first one. Fixed!

calbach · 2020-02-26T22:18:34Z

ui/src/app/utils/cluster-initializer.tsx

+          new ClusterInitializationFailedError('Abort signal received during cluster API call',
+            this.currentCluster));
+      } else if (this.isNotFoundError(e)) {
+        // A not-found error is somewhat expected, if a cluster has recently been deleted or


It does expose that information via getClusters?includeDeleted=true. However, whether or not it was deleted is irrelevant here. In either situation we're going to create a new cluster.

calbach · 2020-02-26T22:21:13Z

ui/src/app/utils/cluster-initializer.tsx

+      if (this.isClusterErrored()) {
+        // TODO: report errors to Stackdriver here.
+        console.log('Cluster error -- deleting for retry');
+        await this.deleteCluster();


Yes makes sense, I surmised as much from the changes - thanks.

…it flow.

…s, add more tests.

calbach reviewed Feb 14, 2020

View reviewed changes

gjuggler changed the title ~~[RW-2704][risk=low] Split up listCluster API and move recovery to the client-side.~~ [RW-2704][risk=low] Split up listCluster API and move initialization logic to the client-side. Feb 21, 2020

gjuggler requested review from jaycarlton and calbach February 21, 2020 15:07

gjuggler marked this pull request as ready for review February 21, 2020 15:10

gjuggler force-pushed the gj/cluster-api branch from 2d059a1 to 67ab8fb Compare February 21, 2020 15:30

jaycarlton reviewed Feb 21, 2020

View reviewed changes

calbach reviewed Feb 22, 2020

View reviewed changes

gjuggler commented Feb 25, 2020

View reviewed changes

gjuggler force-pushed the gj/cluster-api branch from 67ab8fb to 103042c Compare February 25, 2020 01:01

gjuggler requested review from calbach and jaycarlton February 25, 2020 18:53

jaycarlton approved these changes Feb 26, 2020

View reviewed changes

calbach approved these changes Feb 26, 2020

View reviewed changes

gjuggler added 12 commits February 27, 2020 16:42

Work in progress on API refactor and unified client-side polling / in…

6a26dd3

…it flow.

WIP fleshing out cluster retry loop.

2bf95e8

Add and fix unit & controller tests.

62b03f7

A few more text fixes.

9d3001e

Incorporate initializer into reset-cluster-button, use jest.fakeTimer…

0587b2a

…s, add more tests.

Fix API tests.

0fa7a7a

Add some class comments.

87439e1

Fix ng lint error

e84ca22

PR feedback.

3bc2e4b

Spotless formatting and test fix.

4b21497

Fix ts lint

560306b

Revert unintentional change to DataSetController (bad rebase).

59a604b

Final PR feedback.

cf112c3

gjuggler force-pushed the gj/cluster-api branch from 10469de to cf112c3 Compare February 27, 2020 21:55

gjuggler merged commit efc5fa2 into master Feb 27, 2020

gjuggler deleted the gj/cluster-api branch February 27, 2020 22:12


		FirecloudWorkspace fcWorkspace;
		FirecloudWorkspace firecloudWorkspace;

		@@ -1,5 +1,7 @@
		package org.pmiops.workbench.notebooks;

		import static org.pmiops.workbench.exceptions.ExceptionUtils.convertNotebookException;

[RW-2704][risk=low] Split up listCluster API and move initialization logic to the client-side. #3135

[RW-2704][risk=low] Split up listCluster API and move initialization logic to the client-side. #3135

Conversation

gjuggler commented Feb 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjuggler commented Feb 21, 2020

jaycarlton Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaycarlton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaycarlton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjuggler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjuggler commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjuggler commented Feb 14, 2020 •

edited

Loading

jaycarlton Feb 21, 2020 •

edited

Loading