Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

break AWSService interface into smaller pieces (batch, log, etc) #271

Closed
wants to merge 3 commits into from

Conversation

CampGareth
Copy link
Contributor

@CampGareth CampGareth commented Sep 7, 2018

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Description

This PR depends on pingproto being vendored, that's covered by this PR: #265

The implementations involved depend on no_idle, #272

TODO

  • Add locks to fake-batch logs so deletion and streaming from docker cannot happen at the same time
  • Check container has started before streaming logs

Previously we had the AWS interface under service/aws. It was quite big, it covered all of our interactions with AWS and used one implementation with one client for all of this. Now that we're gearing up for on-prem there's a need to have different aspects of that interface handled by different clients, for instance storage goes to minio while batch jobs go to fake-batch.

This PR aims to break up the AWS interface into Batch, Storage and LogStreaming. This should increase flexibility. It also aims to have implementations for these new interfaces so nothing breaks during the transition. Most of the Batch implementation is the same as it was, likewise the Storage, LogStreaming for AWS has been improved and LogStreaming for fake-batch has been created.

cw_id_watcher has been moved since it was the sole user of a function that added complexity to the AWS interface and didn't fit in the new order. That function got moved up to the cron worker level.

Review Checklist

Goals

  • Does it solve the problem?

  • Is it the simplest implementation of that solution?
    Does it yak shave? Does it introduce new dependencies that aren't necessary?

  • Does it decrease modularity?
    Does the user of a module need to import another module to use this one?
    If we want to delete these changes, how easy is that?

  • Does it clarify our domain?
    What things does it refine? What things get added? How does this pave the way for new things?
    Are things named in such a way that a domain expert can find them?

  • Does it introduce non-domain concepts?
    What does the user of this need to learn outside of our domain in order to use this?

Testing

  • Do we integration test changes to external services?

  • Do we unit test code we can change?

response, err := http.Get(URL)
if err != nil {
fmt.Printf("%s", err)
w.Close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be CloseWithError(err), and then don't print the error - if you were printing the error you should use log instead of fmt.

@CampGareth CampGareth force-pushed the feature/batch-interface branch 2 times, most recently from e89193f to 7a1b33b Compare September 18, 2018 13:54
@pwaller pwaller changed the title create batch interface break AWSService interface into smaller pieces (batch, log, etc) Sep 19, 2018
@CampGareth CampGareth force-pushed the feature/batch-interface branch from 1c48e07 to e07409a Compare September 24, 2018 12:42
@CampGareth
Copy link
Contributor Author

fuuu, I missed a commit when fixing the history. fake batch fixes is in there twice as d253cfc and af731e8

@pwaller pwaller force-pushed the feature/batch-interface branch from f132b15 to 3ce4190 Compare October 2, 2018 10:45
@pwaller pwaller force-pushed the feature/batch-interface branch 3 times, most recently from ef75118 to c688e1c Compare October 10, 2018 15:38
pwaller added a commit that referenced this pull request Oct 17, 2018
These are non-semantic changes only, in an effort to reduce the size of
the 'integrate fake-batch' branch in #271.

The intent is that changes in this diff should be 'obviously correct' by
diff inspection and not require much testing.
@pwaller pwaller force-pushed the feature/batch-interface branch from 9f26d34 to df220d1 Compare October 17, 2018 15:50
@pwaller
Copy link
Contributor

pwaller commented Oct 17, 2018

OK, so I worked on this quite a bit this week, and I achieved two things:

  1. I shrank the patch, it's now "only" 200 lines. I did this mostly by:
  1. Got very stuck on how to test this without pushing to production. That lead me to write Move towards "clean architecture" #290 ("Move towards clean architecture").

So, what remains to be done in this PR?

  • Introduce the new batch package and interface. Anyone who wants to start jobs should be calling a method on that interface (not on the concrete type which currently has the name AWS).
  • The await() function, used in the copy logs function, contains a significant piece of missing functionality which means that it will block forever, currently - it doesn't actually query the database to find out the latest state of the job. It's a good candidate for some new methods on the BuildRepo, or something similar.
  • Migrate to CopyLogs().
  • Finally, the thing currently called aws.Service can hopefully become awsbatch.Service or something similar.
  • The currently existing stream primitives should be dropped, since they will no longer be used.

I suggest doing this not by keeping the current WIP patch, but instead by doing it one bit a time and graudally reducing this patch to nothing.

@CampGareth
Copy link
Contributor Author

CampGareth commented Nov 13, 2018

My mind has lost track of what's to be done here so it's time to make a list:

  • Use new log streaming interfaces? I guess?
  • Simplify Batch interface.

@CampGareth
Copy link
Contributor Author

Alright, it's hard to see what actually needs doing but the original goal of this PR was to make fake-batch, on-prem etc. work. In light of that I'm going to do some offline end to end testing and anywhere that doesn't 'just work' I'm going to create a github issue for. Hopefully with all the groundwork that's been laid this shouldn't be too difficult.

@CampGareth CampGareth closed this Nov 13, 2018
@CampGareth CampGareth deleted the feature/batch-interface branch November 13, 2018 14:14
@CampGareth CampGareth removed their assignment Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants