Import resiliency (3/3) #1385

BenEmdon · 2018-07-10T21:39:09Z

What

This PR creates a new user flow that utilizes the new import resiliency changes made in #1368 and #1378 for the purpose of solving #1253.

This particular PR proposes a couple of significant additions:

Registers CreateGitHubRepositoryJob's and PorterStatusJob's queues with sidekiq
Configures ActionCable with our Redis setup
Added a status column to AssignmentInvitation
Introduces a new import_resiliency feature 🚩
The following routes on AssignmentInvitationController:
- GET #progress - JSON api that returns the status of an invitation
- POST #create_repo - if the invitation is accepted, kicks off a CreateGitHubRepositoryJob
- GET #setupv2 - renders a live updating view of the repo creation progress

All of the new repo creation flow is hiding behind the import_resiliency feature 🚩

ActionCable

All the server work for ActionCable is done. The client side work will require some extra thought so I have left that work for a follow up PR since this is already an 🐘. Instead of ActionCable we are using short polling and hitting a GET #progress for now.

What if it errors?

Right now if it errors the UI will reflect the error state but I am yet to add retry button. There is a route ready for the retry button but I have a question about unique/locking background jobs.

What does it look like?

Fix assumption that all assignments need to push starter code

Added spec

BenEmdon · 2018-07-10T21:41:36Z

app/controllers/assignment_invitations_controller.rb

+ if import_resiliency_enabled?
+ job_started = false
+ if current_invitation.accepted? || current_invitation.errored?
+ AssignmentRepo::CreateGitHubRepositoryJob.perform_later(current_assignment, current_user)


Should we ensure that the job is a unique job before we kick it off? Since jobs retry with exponential backoff we could end up in a situation where another job is kicked off while another is about to be retired.

If possible, yes.

By default Sidekiq doesn't have this feature. It does in enterprise, but we aren't on that licensed plan.

Another option would be to use mhenrixon/sidekiq-unique-jobs. This would require us to move away from ActiveJob and to using straight Sidekiq.

Possibly worth investigating in a follow up PR.

BenEmdon · 2018-07-10T21:43:16Z

app/jobs/assignment_repo/create_github_repository_job.rb

+ RepositoryCreationStatusChannel.channel(user_id: user.id),
+ text: Creator::REPOSITORY_CREATION_COMPLETE,
+ status: assignment.invitation&.status
+ )


Should we be recording stats about successful repo creation on this vs the old way? if so using GitHubClassroom.statsd?

Definitely, we should be recording metrics wherever we think might help us be confident the new porter logic is better and more successful than the old one. I said this in an earlier comment, but we'll also want to have granular metrics so we can distinguish imports made using old logic and new logic.

BenEmdon · 2018-07-10T21:44:36Z

config/sidekiq.yml

+ - [deadline, 3]
+ - [boom, 3]
+ - [create_repository, 3]
+ - [porter_status, 3]


Worth mentioning that this many custom queues could be inefficient (#1381 )

d12

I left some comments, mostly everything looks 👍, I think the biggest thing I was unsure about was this new create action. I left some inline comments, let me know what you think.

d12 · 2018-07-11T14:32:28Z

app/assets/javascripts/setupv2.js

+ $.ajax({type: "GET", url: path}).done(function(data) {
+
+ display_progress(data);
+ setTimeout(check_progress, 3000);


The JS here looks good 👍

The only thing I'd suggest is abstracting the two uses of 3000 out to a constant at the top of the file so that if we want to adjust the polling rate in the future, we don't need to go through the whole file looking for where it is.

d12 · 2018-07-11T14:32:49Z

app/assets/javascripts/setupv2.js

+ const path = progress_path();
+
+ $.ajax({type: "GET", url: path}).done(function(data) {
+


style nit, do we need a newline here?

d12 · 2018-07-11T14:32:56Z

app/assets/javascripts/setupv2.js

+
+ start_job = function() {
+ const path = job_path();
+


Same style nit here

d12 · 2018-07-11T14:35:46Z

app/controllers/assignment_invitations_controller.rb

+ result = current_invitation.redeem_for(current_user, import_resiliency: import_resiliency_enabled?)
+ case result.status
+ when :success
+ GitHubClassroom.statsd.increment("exercise_invitation.accept")


Instead of re-using the old metric key here, what do you think about using a new one? v2_excercise_invitation.accept, for example.

This'll give us a little bit more granularity in our metrics when trying to see how the new flow is performing.

d12 · 2018-07-11T14:35:55Z

app/controllers/assignment_invitations_controller.rb

+ GitHubClassroom.statsd.increment("exercise_invitation.accept")
+ redirect_to setupv2_assignment_invitation_path
+ when :error
+ GitHubClassroom.statsd.increment("exercise_invitation.fail")


Same comment about metric granularity here

d12 · 2018-07-11T14:42:18Z

app/controllers/assignment_invitations_controller.rb

+ if import_resiliency_enabled?
+ job_started = false
+ if current_invitation.accepted? || current_invitation.errored?
+ AssignmentRepo::CreateGitHubRepositoryJob.perform_later(current_assignment, current_user)


If possible, yes.

d12 · 2018-07-11T14:46:42Z

app/jobs/assignment_repo/create_github_repository_job.rb

@@ -19,32 +19,25 @@ class CreateGitHubRepositoryJob < ApplicationJob
 def perform(assignment, user)
 start = Time.zone.now

+ assignment.invitation&.creating_repo!


Where is this method defined? I wasn't able to find it.

Ah it's an enum helper, nevermind!

d12 · 2018-07-11T14:51:31Z

app/jobs/assignment_repo/create_github_repository_job.rb

+ RepositoryCreationStatusChannel.channel(user_id: user.id),
+ text: Creator::REPOSITORY_CREATION_COMPLETE,
+ status: assignment.invitation&.status
+ )


Definitely, we should be recording metrics wherever we think might help us be confident the new porter logic is better and more successful than the old one. I said this in an earlier comment, but we'll also want to have granular metrics so we can distinguish imports made using old logic and new logic.

d12 · 2018-07-11T14:53:57Z

app/models/assignment_invitation.rb

+ #
+ # rubocop:disable MethodLength
+ # rubocop:disable AbcSize
+ def redeem_for(invitee, import_resiliency: false)


Instead of using a kwarg here, can we just directly check the feature flag?

I can't use the feature dependency here because we are outside of a controller (feature 🚩s are defined on ApplicationController).

d12 · 2018-07-11T14:59:23Z

db/migrate/20180706215523_add_status_to_assignment_invitation.rb

@@ -0,0 +1,5 @@
+class AddStatusToAssignmentInvitation < ActiveRecord::Migration[5.1]
+ def change
+ add_column :assignment_invitations, :status, :integer


The migration will need to be split into it's own PR, would you mind doing that and pinging me on it? The diff should be this file + the schema diff and nothing else.

d12 · 2018-07-11T15:11:46Z

Also one other thing I'd suggest, this PR is pretty big and it's hard to review and be sure it's 100% correct. If it's possible in the future, I'd recommend breaking PRs into more atomic, smaller changes 😅

houndci-bot · 2018-07-11T22:51:05Z

spec/controllers/assignment_invitations_controller_spec.rb

+ .and_return(result)
+
+ patch :accept, params: { id: invitation.key }
+ end


Layout/BlockAlignment: end at 143, 4 is not aligned with it "sends an event to statsd" do at 135, 6.

BenEmdon · 2018-07-12T21:30:22Z

Update

Changed #create -> #create_repo
Moved poll interval out into a constant
Merged ActionCable fix in Configure Redis adapter for ActionCable #1389
Merged migration in Add status column to assignment_invitation #1386

Things to do in followup PRs

Fix concurrency issue with CreateGitHubRepositoryJob
Add support for group assignments
Support ActionCable instead of short polling

Can I get some final 👀 on this? @d12

BenEmdon added 21 commits July 3, 2018 11:25

Fixed actioncable consumer logic

9c9914b

Added custom job queues

f8401ee

Configured Redis for ActionCable

f59a44d

Add status column to invitation

6ef3480

Broadcast hash structured objects

83d09b1

Fix assumption that all assignments need to push starter code

Added :pending status and made status readable

b416808

Changed invitation status to an ActiveRecord enum

9f8a77c

Added new behavior behind beta flag

10f21be

added progress route

2c602f1

Added front end with polling

2512e81

gave progress route the ability to retry the job on error

baeca08

Update broacasting with status field

90bf12d

Fixed create_github_repository_job spec by adding status to broadcasts

e6fa568

Added #create route to kick off repo creation

54d035c

Added spec

Added setupv2 spec

5178c4f

Please the hound

2de8ff1

Fix cassets

e2e5774

Merge branch 'master' into new-repo-creation-flow

33ae03e

Fix cassets (fr this time)

7d0a98e

Please the hound

2514169

Please hound

addbf3d

BenEmdon requested a review from d12 July 10, 2018 21:39

BenEmdon commented Jul 10, 2018

View reviewed changes

BenEmdon added the 🚀 ready for review label Jul 10, 2018

d12 reviewed Jul 11, 2018

View reviewed changes

BenEmdon mentioned this pull request Jul 11, 2018

Add status column to assignment_invitation #1386

Merged

BenEmdon added 3 commits July 11, 2018 15:49

Removed extra newlines

109a9e0

Added extra stat recording

3fb3909

Added specs for stat reporting

3ab5158

houndci-bot reviewed Jul 11, 2018

View reviewed changes

BenEmdon added 3 commits July 11, 2018 15:53

Please the hound

3cde0ff

Roll back migration in favor of #1386

bbb7eda

Merge branch 'master' into new-repo-creation-flow

15a110d

BenEmdon mentioned this pull request Jul 12, 2018

Configure Redis adapter for ActionCable #1389

Merged

BenEmdon added 4 commits July 12, 2018 09:55

Merge branch 'master' into new-repo-creation-flow

6700963

Remove redundant poll

eb1d7c9

Moved poll interval out into a constant

0ebd551

Change #create to #create_repo

945f5de

Merge branch 'master' into new-repo-creation-flow

7a06406

d12 approved these changes Jul 12, 2018

View reviewed changes

BenEmdon had a problem deploying to production July 12, 2018 21:44 Failure

BenEmdon had a problem deploying to production July 12, 2018 21:46 Failure

BenEmdon added 2 commits July 12, 2018 14:51

Revert to ES5

ca3081f

Remove eslint rule

3c6ecf4

BenEmdon deployed to production July 12, 2018 21:57 Active

Hit /create_repo from js

cc81287

BenEmdon deployed to production July 12, 2018 22:28 Active

BenEmdon merged commit 031ed7a into master Jul 12, 2018

BenEmdon deleted the new-repo-creation-flow branch July 12, 2018 22:31

This was referenced Jul 12, 2018

Import resiliency for group assignments #1394

Closed

Use ActionCable instead of short polling in setupv2 #1395

Closed

Add a manual Retry button to new repo creation flow #1393

Closed

Fix concurrency issue with CreateGitHubRepositoryJob #1392

Closed

BenEmdon mentioned this pull request Jul 18, 2018

Fix ensure_submission_repository_exists for import_resiliency beta #1413

Merged

BenEmdon added the 🚀 ready for review label Oct 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import resiliency (3/3) #1385

Import resiliency (3/3) #1385

BenEmdon commented Jul 10, 2018 •

edited

Loading

BenEmdon Jul 10, 2018

d12 Jul 11, 2018

BenEmdon Jul 11, 2018 •

edited

Loading

BenEmdon Jul 10, 2018

d12 Jul 11, 2018

BenEmdon Jul 10, 2018

d12 left a comment

d12 Jul 11, 2018

d12 Jul 11, 2018

d12 Jul 11, 2018

d12 Jul 11, 2018

d12 Jul 11, 2018

d12 Jul 11, 2018

d12 Jul 11, 2018

d12 Jul 11, 2018

d12 Jul 11, 2018

d12 Jul 11, 2018

BenEmdon Jul 11, 2018 •

edited

Loading

d12 Jul 11, 2018

d12 commented Jul 11, 2018

houndci-bot Jul 11, 2018

BenEmdon commented Jul 12, 2018

		const path = progress_path();

		$.ajax({type: "GET", url: path}).done(function(data) {

Import resiliency (3/3) #1385

Import resiliency (3/3) #1385

Conversation

BenEmdon commented Jul 10, 2018 • edited Loading

What

ActionCable

What if it errors?

What does it look like?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenEmdon Jul 11, 2018 • 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

d12 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

BenEmdon Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d12 commented Jul 11, 2018

Choose a reason for hiding this comment

BenEmdon commented Jul 12, 2018

Update

Things to do in followup PRs

BenEmdon commented Jul 10, 2018 •

edited

Loading

BenEmdon Jul 11, 2018 •

edited

Loading

BenEmdon Jul 11, 2018 •

edited

Loading