Skip to content

Conversation

@a-canary
Copy link
Collaborator

A step towards H2-to-Origin.
This contains minimal change to compile, and a few TODO comments. This will be followed by many PRs to refactor and cleanup methods and variables.

@a-canary a-canary force-pushed the Http1ss_as_ProxySsn branch 3 times, most recently from 61a40cd to 633dec1 Compare August 30, 2019 19:07
@a-canary
Copy link
Collaborator Author

Ready for review @shinrich @masaori335 @maskit @ZhangZizhong

@maskit
Copy link
Member

maskit commented Sep 2, 2019

I think the clang-format should be done by a separate PR because it would make backporting complicated.

@maskit
Copy link
Member

maskit commented Sep 3, 2019

clang-format was done by #5902. Please remove the commit.

{
// TODO: refactor this
ink_assert(0);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this a functional intermediate step? I assume the original versions of these methods are still around and are being used instead of the new stubs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These stubs are the undefined virtual functions of ProxySession. Some existing functions matched the name but not the signature.
Yes, this an intermediate step. Note the "// TODO: refactor this"

@shinrich shinrich added this to the 10.0.0 milestone Sep 3, 2019
A step towards H2-to-Origin.
This contains minimal change to compile, and a few TODO comments. This will be followed by many PRs to refactor and cleanup methods and variables.
+


fix overrides


+
@a-canary a-canary force-pushed the Http1ss_as_ProxySsn branch from 633dec1 to 4da68e6 Compare September 3, 2019 21:42
@a-canary
Copy link
Collaborator Author

a-canary commented Sep 3, 2019

removing format and rebasing

Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

This contains minimal change to compile, and a few TODO comments. This will be followed by many PRs to refactor and cleanup methods and variables.

I'm ok with this change, but I'm a bit worried about how long this journey takes.

  • Does / Should the refactoring block 9.0 release?
  • Can we release 9.0 in the middle of refactoring? (Will master be in good shape on each step?)

@a-canary
Copy link
Collaborator Author

a-canary commented Sep 4, 2019

  • Does / Should the refactoring block 9.0 release?
    No. I don't intend to change the TSAPI's so these refactors should continue after 9.0 is released and backported to 9.x
  • Can we release 9.0 in the middle of refactoring? (Will master be in good shape on each step?)
    My goal is to keep the PRs small scope and frequent, without breaking anything in each PR. So I'm relying on the CI and unit test a lot to get the changes through. To proceed in this manner without breaking anything, I will take extra steps (like stubbing in functions with TODO), and refactor one dependency at a time.

At the moment I have 4 commits queued up for separate PRs. Because like you said, I want the CI to verify each commit to keep the master branch stable.

@maskit
Copy link
Member

maskit commented Sep 5, 2019

No. I don't intend to change the TSAPI's so these refactors should continue after 9.0 is released and backported to 9.x

This is up to RM, but basically backports to 9.0.x are only open for bug fixes once 9.0.0 has been released, 9.1 can have other changes though.

I don't think passing tests is enough. Intermediate state can unnecessarily increase complexity. At the moment, Http1ServerSession doesn't need to extend ProxySession. To people who don't know the plan, this doesn't make sense. IMO, it's fine on feature branches, but not on release branches.

Why don't we have a feature branch for this refactoring?

@a-canary
Copy link
Collaborator Author

a-canary commented Oct 4, 2019

Ok. i'm going to close this PR and open merge this into a feature branch called h1outbound.

@a-canary a-canary closed this Oct 4, 2019
@bryancall bryancall removed this from the 10.0.0 milestone Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants