Skip to content

Conversation

@ab111404212
Copy link

@ab111404212 ab111404212 commented Feb 9, 2023

fix byte array overflow when column id greater than 128

What problem does this PR solve?

when use tidb-cdc in flink,if table column size greater than 128 fields,the left field value cant't be read,and is null value,after review related code,i found a bug, when column id greater than 128,colIds is overflow,so binary search can't find correct column id in colIDs array

the detail bug description
apache/flink-cdc#1883

@ab111404212
Copy link
Author

fix byte array overflow when column id greater than 128

What problem does this PR solve?

when column id greater than 128,binary search can't find column id in colIDs array

@guliangliangatpingcap @birdstorm please help me with review the code

birdstorm
birdstorm previously approved these changes Feb 9, 2023
Copy link
Collaborator

@birdstorm birdstorm left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.02 🎉

Comparison is base (bcd11f3) 37.93% compared to head (b74448a) 37.95%.

❗ Current head b74448a differs from pull request most recent head 8925e1f. Consider uploading reports for the commit 8925e1f to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #717      +/-   ##
============================================
+ Coverage     37.93%   37.95%   +0.02%     
+ Complexity     1610     1609       -1     
============================================
  Files           278      278              
  Lines         17482    17482              
  Branches       1986     1986              
============================================
+ Hits           6631     6635       +4     
+ Misses        10194    10188       -6     
- Partials        657      659       +2     
Impacted Files Coverage Δ
src/main/java/org/tikv/common/codec/RowV2.java 0.00% <0.00%> (ø)
src/main/java/org/tikv/common/pd/PDError.java 0.00% <0.00%> (-30.31%) ⬇️
...java/org/tikv/common/operation/PDErrorHandler.java 40.00% <0.00%> (-20.00%) ⬇️
src/main/java/org/tikv/common/PDClient.java 56.88% <0.00%> (-0.23%) ⬇️
...rc/main/java/io/grpc/netty/NettyClientHandler.java 54.95% <0.00%> (+0.86%) ⬆️
src/main/java/io/grpc/stub/ClientCalls.java 48.51% <0.00%> (+2.31%) ⬆️
src/main/java/org/tikv/common/region/TiStore.java 61.53% <0.00%> (+5.12%) ⬆️
...va/org/tikv/common/region/StoreHealthyChecker.java 69.62% <0.00%> (+8.86%) ⬆️
src/main/java/org/tikv/txn/TxnExpireTime.java 73.68% <0.00%> (+10.52%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: wuyj@shuyilink.com
@ab111404212
Copy link
Author

@birdstorm amend commit information,please review again

Copy link
Collaborator

@birdstorm birdstorm left a comment

Choose a reason for hiding this comment

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

LGTM

@shiyuhang0
Copy link
Collaborator

Please add a test if it is convenient

@shiyuhang0
Copy link
Collaborator

Fix DCO please

@ti-srebot
Copy link
Collaborator

@shiyuhang0, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. get siglist: dial tcp 172.16.4.167:34000: connect: no route to host

@ti-srebot
Copy link
Collaborator

@zhangyangyu, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. get siglist: dial tcp 172.16.4.167:34000: connect: no route to host

@zhangyangyu
Copy link
Contributor

@ab111404212 could you fix the DCO check?

@shiyuhang0
Copy link
Collaborator

shiyuhang0 commented Mar 7, 2023

Your signoff can not match your GitHub mail causing the DCO fail.
img_v2_6502bac6-c5c8-4b83-8429-14e9c084619g

we are going to release a new version soon and want to include this fix. so we close your PR and open a new one. Thanks for your contribution!

@shiyuhang0 shiyuhang0 mentioned this pull request Mar 7, 2023
@shiyuhang0
Copy link
Collaborator

closed by #728

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.

5 participants