-
Notifications
You must be signed in to change notification settings - Fork 504
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
Upgrades Finagle version to 18.12.0 #2194
Conversation
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From now on whenever I type com.twitter.conversions.time._
it will be a Duration Oops. 🤣
I have one question about reader. Otherwise this looks good.
def read(): Future[Option[Buf]] = reader.read().handle { | ||
case NonFatal(e) => | ||
log.debug(e, "Error reading JSON stream") | ||
None | ||
} | ||
def discard(): Unit = reader.discard() | ||
|
||
def onClose: Future[StreamTermination] = closeP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this promise is never satisfied? why shouldn't this be a pass through to reader.onClose
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @adelong's comment about onClose, lgtm otherwise. Glad the netty fix is resolved now :)
@@ -5,8 +5,7 @@ import com.fasterxml.jackson.annotation._ | |||
import com.fasterxml.jackson.core.{JsonParser, TreeNode} | |||
import com.fasterxml.jackson.databind.annotation.JsonDeserialize | |||
import com.fasterxml.jackson.databind.{DeserializationContext, JsonDeserializer, JsonNode} | |||
import com.twitter.conversions.storage._ | |||
import com.twitter.conversions.time._ | |||
import com.twitter.conversions.DurationOps._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we ran into this issue too. It should be fixed in the next release: twitter/util#239
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh awesome, glad this was fixed. 😄
import com.twitter.conversions.time._ | ||
import com.twitter.finagle.{Path, Stack} | ||
import com.twitter.conversions.StorageUnitOps._ | ||
import com.twitter.conversions.DurationOps._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these imports used? The issue I linked should prevent them from working...
Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐ 🎅
Ah, looks like I can't merge it without @ccmtaylor's approval. I was wondering why I couldn't see the big green button haha. |
fixes #2180
fixes #2184