Fixes int overflow issue for large TTL values #3367
Conversation
|
Ping @anuraaga
…On Sat, 14 Aug 2021, 22:16 m, ***@***.***> wrote:
Fixes #3357 <#3357>.
The line indexTtl * 1000 will produce an integer overflow for large
indexTtl values. For example 7890000 * 1000 will produce an integer
overflow (some negative number).
The simple fix is to use long in the multiplication.
------------------------------
You can view, comment on, or merge this pull request online at:
#3367
Commit Summary
- Fixes int overflow issue for large TTL values
File Changes
- *M*
zipkin-storage/cassandra/src/main/java/zipkin2/storage/cassandra/CassandraSpanStore.java
<https://github.com/openzipkin/zipkin/pull/3367/files#diff-d85e858924df5caed00b20b67deac8389b768227fc7fdf8f7575884ae941a663>
(2)
Patch Links:
- https://github.com/openzipkin/zipkin/pull/3367.patch
- https://github.com/openzipkin/zipkin/pull/3367.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3367>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYATCSMZBLEHKGXAOGLTT43FLVANCNFSM5CFNDVLA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
@vy8 could you add a unit test for this? |
|
Hey @jcchavezs just did. Even thought it's a simple test that checks if startMillis is smaller than endMillis it will fail with the old code that doesn't have the explicit 'L' for long. |
| @@ -305,8 +305,8 @@ | |||
| UUID endUUID; | |||
| } | |||
|
|
|||
| TimestampRange timestampRange(QueryRequest request) { | |||
| long oldestData = Math.max(System.currentTimeMillis() - indexTtl * 1000, 0); // >= 1970 | |||
| TimestampRange timestampRange(QueryRequest request, int indexTtl) { | |||
There was a problem hiding this comment.
Do we truly need to pass indexTtl as param? I see this is a class attribute: https://github.com/openzipkin/zipkin/pull/3367/files#diff-d85e858924df5caed00b20b67deac8389b768227fc7fdf8f7575884ae941a663L54
There was a problem hiding this comment.
@jcchavezs the indexTtl value comes from a static method and then gets set to a final class attribute. To create a test I need to either:
a) Set the property using reflection for the test.
b) Mock a static method.
c) Pass the indexTtl as a param.
I chose c) since it seemed as the least amount of hassle. Tell me which option you would prefer and I'll implement it.
Cheers!
|
Somehow the test is failing. Would you please look into it? |
vy8
force-pushed
the
fix-int-overflow
branch
from
d88d40b to
7c5d761
Compare
|
@jcchavezs bumped the year in the license from 2020 to 2021 and all should be good. |
|
thanks. will be in next release |
Fixes #3357.
The line
indexTtl * 1000will produce an integer overflow for large indexTtl values. For example 7890000 * 1000 will produce an integer overflow (some negative number).The simple fix is to use long in the multiplication.