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

Support unsigned integer #618

Closed

Conversation

mehmetgunturkun
Copy link
Contributor

Problem

finagle-mysql does not support unsigned integers. If an integer field is with unsigned attribute, it has larger maximum limit (4294967295) than signed integer. Since there is no support for unsigned integer in Java, if the value is higher than Integer.MAX_VALUE (2147483647), it is interpreted as IntValue(-1)

Solution

In order to understand the field is unsigned or not, I added bit masks for type attributes and a function to Field class. Bit masks for attributes are listed in the source code of mysql (https://github.com/mysql/mysql-server/blob/5.7/include/mysql_com.h). During field decoding, if the field is unsigned, I created a LongValue and used readUnsignedIntLE function from MysqlBufReader.

Additionally, added an integration test case into com.twitter.finagle.mysql.integration.TypeTest.NumericTypeTest. Added unsigned int field, inserted an integer value higher than Integer.MAX_VALUE, and expected to be parsed as LongValue.

Copy link
Contributor

@mosesn mosesn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks great so far.

@@ -146,6 +146,23 @@ object EOF extends Decoder[EOF] {
case class EOF(warnings: Short, serverStatus: ServerStatus) extends Result

/**
* These bit masks are to understand whether corresponding attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be styled like:

/**
 *
 */

case Type.Tiny => ByteValue(str.toByte)
case Type.Short => ShortValue(str.toShort)
case Type.Int24 => IntValue(str.toInt)
case Type.Long if field.isUnsigned() => LongValue(str.toLong)
Copy link
Contributor

Choose a reason for hiding this comment

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

this won't work for longs bigger than 2^63, right? should we add a field to LongValue that we're encoding an unsigned long in a signed long? or add a new type, UnsignedLong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't get. Longs bigger than 2^63 has Type.LongLong instead of Type.Long. Should we use isUnsigned check for Type.LongLongs too?

Copy link
Contributor

Choose a reason for hiding this comment

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

well, unsigned longs have the same number of bits as signed longs, they're just encoded differently, so we could conceivably encode them as signed longs in scala. it's just a design decision. I don't think it would make sense to make them LongLongs, since we know it can't be most of the numbers in LongLongs. yeah, it would be nice to use isUnsigned for all of the types that could be unsigned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok then, just to clarify. I am using isUnsigned for Type.LongLong too and add another test case for that, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

not exactly. suppose mysql wants to return an unsigned long of 9223372036854775808 (as we talked about below). "9223372036854775808".toLong fails. How do we do this properly? Note that mysql will consider it a long, not a longlong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah and if the data type of the column in MySQL, is unsigned long then the type of the field in finagle side is Type.LongLong.

BTW by saying

The column should be able to be unsigned long

You mean BIGINT UNSIGNED, right. Is there a data type long n MySQL?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, there's a Long type, that's the field that we match on in this code. although reading more, I think it's actually 4 bytes, not 8 bytes. but I'm really starting to think that all of the numeric values that we get out of mysql, we should annotate whether they're unsigned or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so too because the unsigned logic applies to all numeric values.

Think about smallint if it is unsigned it can go until 65535 in MySQL side, but we are converting it to Short which has 32767 as a MaxValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @mosesn, should I write and send the new version that includes all numerical fields with isUnsigned check?

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me!

@@ -111,6 +112,13 @@ class NumericTypeTest extends FunSuite with IntegrationClient {
case v => fail("expected a RawValue but got %s".format(v))
}
}

test("extract %s from %s".format("unsigned", rowType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a test for a long that's bigger than what scala can fit into a long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sth like: 9223372036854775808 (BigInt(Long.MaxValue) + 1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

that would be perfect =)

Copy link
Contributor

@mosesn mosesn left a comment

Choose a reason for hiding this comment

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

sorry for the delay, I was sick over the weekend. I'm not sure how much I love this API, but since unsigned stuff is probably not the common case (and is difficult to represent in scala), and we can always change it if we have to, it's probably OK. let me get some more eyes on this but I think this should be good to go modulo a few small changes.

case Type.Float => FloatValue(str.toFloat)
case Type.Double => DoubleValue(str.toDouble)
case Type.Year => ShortValue(str.toShort)
case Type.Tiny if field.isSigned() => ByteValue(str.toByte)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a comment as to why we're doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that you're talking about isSigned logic, right? If it is, should I write comments for BinaryEncodedRow too?

Copy link
Contributor

Choose a reason for hiding this comment

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

that would be rad, but I think if you add the helper method you described, that will document it well enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That helper method might need implicit parameters, is that OK? Some project might have rules likes "do not use implicits" so I want to ask.

case Type.Tiny => ShortValue(str.toShort)
case Type.Short if field.isSigned() => ShortValue(str.toShort)
case Type.Short => IntValue(str.toInt)
case Type.Int24 if field.isSigned() => IntValue(str.toInt)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can merge ones where it doesn't matter if it's signed or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I thought it might be good to see all cases in discrete.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like them explicit too, tbh.

@mehmetgunturkun
Copy link
Contributor Author

Hi @mosesn, get well soon.

Yeah, I do not like checking isSigned in each case too. I though maybe adding this kind of function might help but couldn't be sure:

val value = getValueForField[Int, Long](str, field)
val value = getValueForField[Int, Long](reader, field)

so that isUnsigned check would be only in that function.

@mosesn
Copy link
Contributor

mosesn commented May 16, 2017

@mehmetgunturkun I like that, :shipit:

@mehmetgunturkun
Copy link
Contributor Author

Cool, let me change this code into that. Awesome!


private val unsignedLongMaxValue: BigInt = BigInt("18446744073709551615")

def readUnsignedLongLE(): BigInt = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these seem generally useful. Maybe we should move them under ByteReader, where we have O(1) get?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, it is under twitter/util repo, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@roanta roanta left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for patch!

/**
* Retrieves string of the given code.
*/
private[mysql] def getCodeString(code: Short): String = code match {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added for debugging actually but left as is, it might be helpful.

case Type.Tiny => ShortValue(str.toShort)
case Type.Short if field.isSigned() => ShortValue(str.toShort)
case Type.Short => IntValue(str.toInt)
case Type.Int24 if field.isSigned() => IntValue(str.toInt)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like them explicit too, tbh.

@mehmetgunturkun
Copy link
Contributor Author

mehmetgunturkun commented May 23, 2017

Hi @mosesn I tried to do that with the helper function I mentioned
val value = getValueForField[Int, Long](str, field)

It gets too complicated due to the implicit classes, so I guess this is a more clear way to do that, but if you want to see that version I can create another branch for you to review.

@vkostyukov I added this part of the code to ByteReader, but how should I merge that to this? One idea is to wait for twitter/util to release a new version then upgrade its version in twitter/finagle and remove the duplicated part.

@mosesn
Copy link
Contributor

mosesn commented May 23, 2017

@mehmetgunturkun no need for a full review, but do you mind posting your work in a gist? I'm a bit curious.

@mehmetgunturkun
Copy link
Contributor Author

Hi @mosesn here is the link for gist:

https://gist.github.com/mehmetgunturkun/e3f7974819a29bc6cd048eb4366707cc

Check StringValueConverter and BinaryValueConverter

@mosesn
Copy link
Contributor

mosesn commented May 23, 2017

@mehmetgunturkun ah, I see what you mean. LGTM then.

@mehmetgunturkun
Copy link
Contributor Author

@mosesn What about the merge with twitter/util? Are we going to wait for twitter/util to publish changes in ByteReader?

@ryanoneill
Copy link
Contributor

@mehmetgunturkun FYI, I've put up that change for review internally. It required some additional changes to finagle/finagle-netty4 but should be (hopefully) shipped later today.

@mosesn
Copy link
Contributor

mosesn commented May 26, 2017

@mehmetgunturkun this was merged in util twitter/util@94ab01f, so you should be able to take advantage of it now. thanks for your patience!

@mehmetgunturkun
Copy link
Contributor Author

Hi @mosesn Should I wait for new release of twitter/util (probably 6.44.0) to move on to this issue, or is there a quicker way to use develop branch of twitter/util?

@@ -207,6 +223,9 @@ case class Field(
) extends Result {
def id: String = if (name.isEmpty) origName else name
override val toString = "Field(%s)".format(id)

def isUnsigned(): Boolean = (flags & FieldAttributes.UnsignedBitMask) > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Some style nits for ya that I missed earlier:

We typically leave parens off of these types of methods (both defining and calling) unless they mutate in some way. Since this doesn't, please omit here and in isSigned

@mosesn
Copy link
Contributor

mosesn commented May 31, 2017

@mehmetgunturkun you don't have to wait for the new release. the instructions in CONTRIBUTING are for using the tip of util develop branch.

Copy link
Contributor

@nepthar nepthar left a comment

Choose a reason for hiding this comment

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

Thanks!

@mehmetgunturkun
Copy link
Contributor Author

@ryanoneill Was the change, you mentioned, related to CopyingByteBufByteReader.scala?

@ryanoneill
Copy link
Contributor

@mehmetgunturkun yes, via AbstractByteBufByteReader. e62c326

@codecov-io
Copy link

codecov-io commented May 31, 2017

Codecov Report

Merging #618 into develop will decrease coverage by 0.17%.
The diff coverage is 2.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #618      +/-   ##
===========================================
- Coverage     68.2%   68.03%   -0.18%     
===========================================
  Files          634      634              
  Lines        21101    21161      +60     
  Branches      1567     1578      +11     
===========================================
+ Hits         14392    14396       +4     
- Misses        6709     6765      +56
Impacted Files Coverage Δ
...c/main/scala/com/twitter/finagle/mysql/Value.scala 70.29% <ø> (ø) ⬆️
.../main/scala/com/twitter/finagle/mysql/Result.scala 83.96% <0%> (-8.75%) ⬇️
...rc/main/scala/com/twitter/finagle/mysql/Type.scala 34.61% <0%> (-18.33%) ⬇️
...src/main/scala/com/twitter/finagle/mysql/Row.scala 24.59% <3.57%> (-5.41%) ⬇️
...ala/com/twitter/finagle/mysql/CanBeParameter.scala 70.83% <9.09%> (-8%) ⬇️
...c/main/scala/com/twitter/finagle/http/Method.scala 83.33% <0%> (-4.17%) ⬇️
...agle/http2/transport/Http2UpgradingTransport.scala 88.88% <0%> (-3.71%) ⬇️
...nagle/http2/transport/MultiplexedTransporter.scala 82.65% <0%> (+0.57%) ⬆️
.../main/scala/com/twitter/finagle/mysql/Client.scala 60.46% <0%> (+0.94%) ⬆️
...ain/scala/com/twitter/finagle/tracing/Tracer.scala 71.25% <0%> (+1.25%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e23d08...5eb03e5. Read the comment docs.

@mehmetgunturkun
Copy link
Contributor Author

@mosesn I guess I broke something. I just merged develop -> Support_Unsigned_Integer and removed these unnecessary functions, but coverage is decreased. I did sth. wrong?

@bryce-anderson
Copy link
Contributor

@mehmetgunturkun, I have some bad news: we have a few problems. First one is simple: we need a CanBeParameter instance for BigInt.

Second one is not so simple, but not too hard. With your change, most int *Value types are now widened if the DB schema is an unsigned integer type. For example what was a LongValue may be now represented as a BigIntValue if the DB schema says the column is an unsigned long long. This causes things like below to fail:

// Will result in a MatchError
// terrible idea, but it happens and only fails at runtime. :(
val LongValue(foo) = row("foo").get

A workaround for this is to make consideration of signedness configurable with a default of off. In my minds eye you just need to add a flag to the Row implementations and in the match statements do something like this:

field.fieldType match {
...
case Type.Long if considerSign && field.isSigned => 
...

What do you think?

@mehmetgunturkun
Copy link
Contributor Author

mehmetgunturkun commented Jun 5, 2017

@bryce-anderson, I got it, I'm adding that flag.

For CanbeParameter what should be the typeCode for BigIntValue, is Type.LongLong ok:
def typeCode(param: BigInt) = Type.LongLong

@bryce-anderson
Copy link
Contributor

@mehmetgunturkun I think Type.LongLong is correct.

@mehmetgunturkun
Copy link
Contributor Author

Hi @bryce-anderson, sorry for long delay. I have added considerSign flag to Row and take as a parameter in constructors of BinaryEncodedRow and StringEncodedRow, but I am not sure who is going to set this flags.

BTW, if it is not clear, I can push the current version for you to check.

@bryce-anderson
Copy link
Contributor

@mehmetgunturkun, it would be great to see the code. Ultimately this will probably be something that gets set during client construction with a stack Param.

Copy link
Contributor

@nepthar nepthar left a comment

Choose a reason for hiding this comment

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

This approach works for me. Thanks!

@@ -20,6 +20,14 @@ trait Row {
/** The values for this Row. */
val values: IndexedSeq[Value]

/**/
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an unfinished comment.

Fun side note, this broke github's syntax highlighting

@@ -70,16 +78,17 @@ class StringEncodedRow(rawRow: Buf, val fields: IndexedSeq[Field], indexMap: Map
RawValue(field.fieldType, field.charset, false, bytes)
else {
val str = new String(bytes, Charset(charset))
println(field, str)
Copy link
Contributor

Choose a reason for hiding this comment

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

stray print

val signedTextEncodedQuery = """SELECT `tinyint`, `smallint`, `mediumint`, `int`, `bigint`, `float`, `double`,`decimal`, `bit` FROM `numeric` """
runTest(c, signedTextEncodedQuery)(testRow)

// TODO Comment out after ignoreUnsigned = true
Copy link
Contributor

Choose a reason for hiding this comment

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

stray

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean remove TODO part (Line 51) or remove comments for unsigned test cases?

new CanBeParameter[BigInt] {
def sizeOf(param: BigInt) = 8
def typeCode(param: BigInt) = Type.LongLong
def write(writer: MysqlBufWriter, param: BigInt) = writer.writeBytes(param.toByteArray)
Copy link
Contributor

Choose a reason for hiding this comment

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

Assert that the BigInt being written can indeed fit in 8 bytes. (Unless this is verified somewhere else that I can't see)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting checking the size of the array, to be sure that it is shorter than or equal to 8?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, since (AFAICT) BigInt could store values that would be larger than 8 bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if it is larger than 8, take first 8 bytes or throw an exception?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think throwing is the way to go. I'd rather have that then write invalid data.
@bryce-anderson what say you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @nepthar: I think a throw is fine in this case, although I don't know where that exception will surface off hand. Also, I think you need to make sure you write exactly 8 bytes and right now you will write less in the majority of cases:

scala> BigInt(10).toByteArray
res1: Array[Byte] = Array(10) // only one byte!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can create an array of bytes in size of 8 and fill it in little endian order, is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that or just pad with 0's via .writeByte(0x0).

@@ -20,7 +20,7 @@ trait Row {
/** The values for this Row. */
val values: IndexedSeq[Value]

/**/
/** The value is to consider unsigned flag of field or not */
Copy link
Contributor

Choose a reason for hiding this comment

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

haha awesome commit message :)

Sorry, I didn't mean to suggest that a comment was necessary here, only that there was a stray "/**/"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I miswrote that, message should have been "a sane comment" :)

Copy link
Contributor

@nepthar nepthar left a comment

Choose a reason for hiding this comment

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

This looks great, I think we've got all of the basis covered one you switch to a named exception. It'll help stack traces make sense.

val lengthOfByteArray: Int = byteArray.length

if (lengthOfByteArray > 8) {
throw new Exception(s"The length of BigInt is larger than 8 bytes: $lengthOfByteArray > 8")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create a new named exception for this (can define it somehwere in the file here). I suggest something like:
class BigIntTooLongException(size: Int) extends Exception(s"BigInt is stored as an unsigned long and thus cannot be grater than 8 bytes. Size: $size"

@mehmetgunturkun
Copy link
Contributor Author

@bryce-anderson I am trying to merge develop branch into Support_Unsigned_Integer. However, I cannot pass all tests in develop branch. NumericTypeTest.scala fails and when I change readMediumLE to readIntLE, it passes, do you have any idea, why?

@bryce-anderson
Copy link
Contributor

@mehmetgunturkun hard to say for sure without more details, but I don't understand why you would be changing readMediumLE to readIntLE. From the looks of things, there are a few place in this PR where we satisfy an Int24 with readIntLE which seems wrong: won't that consume 4 bytes when we only want to consume 3?

Copy link
Contributor

@bryce-anderson bryce-anderson left a comment

Choose a reason for hiding this comment

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

I don't know if this is what you had concerns with regarding merging master, so maybe its already addressed.

case Type.Tiny => ShortValue(reader.readUnsignedByte())
case Type.Short if isSigned(field) => ShortValue(reader.readShortLE())
case Type.Short => IntValue(reader.readUnsignedShortLE())
case Type.Int24 if isSigned(field) => IntValue(reader.readIntLE())
Copy link
Contributor

Choose a reason for hiding this comment

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

Type.Int24 and TypeInt24 if signed need to call reader.readMediumLE() and reader.readUnsignedMediumLE(), respectively.

# Conflicts:
#	finagle-mysql/src/main/scala/com/twitter/finagle/mysql/Row.scala
@mehmetgunturkun
Copy link
Contributor Author

@bryce-anderson merged develop to this branch, everything seems to be fine.

}

for (i <- 0 until lengthOfByteArray) {
writer.writeByte(byteArray(i))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to reverse the order of the bytes we read out of byteArray: the BigInt.toByteArray method returns the bytes in big-endian order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, sorry I should have checked that - sending the fix.

val signedTextEncodedQuery = """SELECT `tinyint`, `smallint`, `mediumint`, `int`, `bigint`, `float`, `double`,`decimal`, `bit` FROM `numeric` """
runTest(c, signedTextEncodedQuery)(testRow)

// TODO Comment out after ignoreUnsigned = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ready to be uncommented now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly, not sure how to set ignoreUnsigned flag for StringEncodedRow and BinaryEncodedRow.

class StringEncodedRow(rawRow: Buf, val fields: IndexedSeq[Field], indexMap: Map[String, Int], val ignoreUnsigned: Boolean = true) extends Row

class BinaryEncodedRow(rawRow: Buf, val fields: IndexedSeq[Field], indexMap: Map[String, Int], val ignoreUnsigned: Boolean = true)

As you see I set it to true in the constructor, how to set it to false explicitly, especially in a test?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're going to need to make an way to configure the Client to operate in 'support unsigned' mode. This will entail making a Stack.Param and threading the boolean all the way through. Which will be a bit of a pain, but it is what it is. I'd remove the default argument for the Row constructors since you're always going to have to pass it up from a higher level anyway.

Stack.Params can be a little convoluted at times (all times, really), so look at the HTTP implementation for an example here. The basic idea is to put typed 'params' (Stack.Param[T]) into a map where the stored items are typed based on the key (the T) and all params have a default.
It can then be configured using the pattern:

client.configured(SupportUnsignedInts(true))
  .whatever(..)
  .newService(..)

@bryce-anderson
Copy link
Contributor

@mehmetgunturkun, hows it going on this?

@bryce-anderson
Copy link
Contributor

@mehmetgunturkun this has been merged internally and we're just waiting for the github sync process to publish it. Good news is that it should also make it out into pending finagle release.

finaglehelper pushed a commit that referenced this pull request Aug 14, 2017
Summary: This reverts commit ba1438af3971129c7e6c80080efdf669a7fde4e3.

This commit is based on a Github PR, #618, and the authorship wasn't merged
correctly so this will revert the work, then a follow up will revert-the-revert to
fix the authorship.

Differential Revision: https://phabricator.twitter.biz/D80814
@bryce-anderson
Copy link
Contributor

Closed by 598acc5. Thanks again, @mehmetgunturkun.

@mehmetgunturkun
Copy link
Contributor Author

Hey, @bryce-anderson that's great news.
Sorry for my inconvenience, I couldn't pay enough attention to the PR these days but it is awesome that it is merged now.

Thanks for your support too.

mkhq pushed a commit to mkhq/finagle that referenced this pull request Aug 21, 2017
Summary: This reverts commit ba1438af3971129c7e6c80080efdf669a7fde4e3.

This commit is based on a Github PR, twitter#618, and the authorship wasn't merged
correctly so this will revert the work, then a follow up will revert-the-revert to
fix the authorship.

Differential Revision: https://phabricator.twitter.biz/D80814
mkhq pushed a commit to mkhq/finagle that referenced this pull request Aug 24, 2017
Summary: This reverts commit ba1438af3971129c7e6c80080efdf669a7fde4e3.

This commit is based on a Github PR, twitter#618, and the authorship wasn't merged
correctly so this will revert the work, then a follow up will revert-the-revert to
fix the authorship.

Differential Revision: https://phabricator.twitter.biz/D80814
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

8 participants