Skip to content

Add unit tests for activator#1585

Closed
tanzeeb wants to merge 1 commit intoknative:masterfrom
pivotal-cf-experimental:activator-unit-tests
Closed

Add unit tests for activator#1585
tanzeeb wants to merge 1 commit intoknative:masterfrom
pivotal-cf-experimental:activator-unit-tests

Conversation

@tanzeeb
Copy link
Copy Markdown
Contributor

@tanzeeb tanzeeb commented Jul 12, 2018

Refactored retryRoundTripper and activationHandler into multiple
components to make testing simpler.

Fixes #1231

Refactored retryRoundTripper and activationHandler into multiple
components to make testing simpler.

Fixes #1231
@google-prow-robot google-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 12, 2018
@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tanzeeb
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: josephburnett

If they are not already assigned, you can assign the PR to them by writing /assign @josephburnett in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mattmoor
Copy link
Copy Markdown
Member

/hold

Wait until after we cut first release.

@google-prow-robot google-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 12, 2018
@tanzeeb
Copy link
Copy Markdown
Contributor Author

tanzeeb commented Jul 19, 2018

/hold cancel

@google-prow-robot google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 19, 2018
@tanzeeb
Copy link
Copy Markdown
Contributor Author

tanzeeb commented Jul 19, 2018

/assign @josephburnett

@@ -0,0 +1,103 @@
/*
Copyright 2018 The Knative Authors
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@tanzeeb - I didn't see this PR before and I was doing the exact same work on my side - Luckily I was half complete.

Can you take a look at my own version of round tripper refactoring here:
https://github.com/mdemirhan/serving/blob/activatortraces/pkg/activator/httptransport.go
https://github.com/mdemirhan/serving/blob/activatortraces/pkg/activator/httptransport_test.go

I have a biased preference towards using the implementation above for a couple of reasons :-)

  • It instantiates the transport for HTTP2 only once and keeps reusing it. Creating an HTTP2 transport for every transaction is unnecessary
  • It implements the RoundTripper interface on the pointer rather than the value preventing extra memory copies.

Please take a look and if it looks good, feel free to copy it over this one as part of your PR. I will wait for your changes to get in before I add tracing features to activator.

Let me know what you think.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mdemirhan awesome, I'll update the PR!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mdemirhan I've posted updates in #1689 , please let me know what you think.

@tanzeeb
Copy link
Copy Markdown
Contributor Author

tanzeeb commented Jul 25, 2018

Continuing this PR on #1689, since the original fork is a closed source repo.

@tanzeeb tanzeeb closed this Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants