revert grpc breaking netty tls change #200
Conversation
pjfanning
requested review from
He-Pin,
nvollmar,
mdedetrich and
samueleresca
|
adding @mkurz too Since we are now in license hell due to #197, I think the best starting place is to revert the original change that caused the issue (that was made before Pekko forked and we are safe to reference that change since it is an OSS friendly chane). We can build on top of this but this might a starting point. |
| nettySslContextField.set(nettySslContext, javaSslContext) | ||
|
|
||
| nettySslContext |
There was a problem hiding this comment.
so this is leveraging the netty and based on a special impl too, future netty can do some kind of refactoring, and then this will fail again.
There was a problem hiding this comment.
it will work for now and we can build a new fix - code is a living thing, today's fixes can be improved later on
we have broken code today and #197 makes it hard for us to do a better fix than this in the short term
this change gets us back to the working code in akka-grpc 2.1.5
There was a problem hiding this comment.
Nice find, I have not check it:) maybe someone will do some tweets on X then.
And for the fix, I think that should not be hard. the tests suit maybe a little complex which need some keys setup.
Before we come up a fix, we should provide a test about it first.
There was a problem hiding this comment.
Another issue is, can't we just do a quick reset and git push -f to drop that commit?
There was a problem hiding this comment.
Another issue is, can't we just do a quick reset and git push -f to drop that commit?
I'm sure if removing the #197 commit from git history is required but if it the consensus is to try to hide it altogether, we can go that way.
There was a problem hiding this comment.
We should just drop it completly I think, otherwise the result commit log contains incompatible code.
There was a problem hiding this comment.
The thing is that the git branch is protected - it will need to be unprotected to allow commits to be removed. Anyone who has a local git branch with the commit would also need to clean their set up afterwards. So forcing a removal of the commit is pretty drastic. It is not clear that it is absolutely needed.
There was a problem hiding this comment.
I know, but more safer way I think.
There was a problem hiding this comment.
I am against unprotecting the git branch, it may have been acceptable right at the time of forking pekko but at this point there are way too many people that have this git branch checked out and over-writing the main branch basically forces everyone who has git cloned this repo to re-pull the changes, so even if we do remove the commit from main unless people wipe their repo they will still have the commit.
Thing is, I don't know what the ASF policy on this is, if I had a say in this I would add the commit (and the revert commit) to .git-ignore-blame-revs so that its more hidden but realistically I don't know what more can be done.
|
this seems to fix the play-grpc issue - playframework/play-grpc#468 (comment) |
|
Thanks for fixing this over the holidays! I see you just released v1.0.2, so I released play-grpc 0.12.1 as well: |
see #197 and #199
the akka-grpc 2.1.6 breakage was traced back to this OSS akka-grpc 2.1.6 change (akka/akka-grpc#1649) and reverted here