Skip to content

Conversation

@Daemonxiao
Copy link
Contributor

Signed-off-by: Daemonxiao 735462752@qq.com

What problem does this PR solve?

TiSpark met some problem when use TLS Reload.

Problem Description: tryReload() is used as a task in ScheduledFuture. When tryReload() throws an exception, the schedule will be suppressed which makes tryReload() will not work.

What is changed and how does it work?

Add try-catch in tryReload().

    // If any execution of the task encounters an exception, subsequent executions are suppressed.
    private void tryReload() {
      // Add exception handling to avoid schedule stop.
      try {
        if (needReload()) {
          onChange.run();
        }
      } catch (Exception e) {
        logger.error("Failed to reload cert!" + e);
      }
    }

Code changes

  • Has exported function/method change

Check List for Tests

This PR has been tested by at least one of the following methods:

  • Unit test
  • Integration test

Side effects

  • NO side effects

Related changes

  • Need to cherry-pick to the release branch

Signed-off-by: Daemonxiao <735462752@qq.com>
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #609 (fdf0e13) into master (9ce52d3) will increase coverage by 0.02%.
The diff coverage is 76.92%.

@@             Coverage Diff              @@
##             master     #609      +/-   ##
============================================
+ Coverage     34.60%   34.63%   +0.02%     
+ Complexity     1418     1416       -2     
============================================
  Files           278      278              
  Lines         17342    17352      +10     
  Branches       1970     1971       +1     
============================================
+ Hits           6002     6009       +7     
  Misses        10732    10732              
- Partials        608      611       +3     
Impacted Files Coverage Δ
...main/java/org/tikv/common/util/ChannelFactory.java 62.20% <53.84%> (-0.60%) ⬇️
.../org/tikv/common/apiversion/RequestKeyV2Codec.java 82.85% <100.00%> (+1.03%) ⬆️
...g/tikv/common/apiversion/RequestKeyV2RawCodec.java 100.00% <100.00%> (ø)
...g/tikv/common/apiversion/RequestKeyV2TxnCodec.java 100.00% <100.00%> (ø)
...rc/main/java/io/grpc/netty/NettyClientHandler.java 56.89% <0.00%> (-0.22%) ⬇️
...ty/handler/codec/http2/Http2ConnectionHandler.java 49.14% <0.00%> (+0.48%) ⬆️

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 aacbb8c...fdf0e13. Read the comment docs.

sunxiaoguang
sunxiaoguang previously approved these changes Jun 13, 2022
@sunxiaoguang sunxiaoguang dismissed their stale review June 13, 2022 13:23

There is something need to change

@Daemonxiao Daemonxiao force-pushed the fix_tls_reload branch 2 times, most recently from 1b95aab to b563b4b Compare June 14, 2022 01:49
Signed-off-by: Daemonxiao <735462752@qq.com>
Co-authored-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: Daemonxiao <735462752@qq.com>
Co-authored-by: iosmanthus <myosmanthustree@gmail.com>
iosmanthus
iosmanthus previously approved these changes Jun 14, 2022
Copy link
Member

@iosmanthus iosmanthus left a comment

Choose a reason for hiding this comment

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

LGTM

ti-srebot
ti-srebot previously approved these changes Jun 14, 2022
Signed-off-by: Daemonxiao <735462752@qq.com>
Co-authored-by: iosmanthus <myosmanthustree@gmail.com>
@Daemonxiao Daemonxiao dismissed stale reviews from ti-srebot and iosmanthus via e355142 June 14, 2022 07:25
iosmanthus
iosmanthus previously approved these changes Jun 14, 2022
Copy link
Member

@iosmanthus iosmanthus left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Daemonxiao <735462752@qq.com>
Co-authored-by: iosmanthus <myosmanthustree@gmail.com>
Signed-off-by: Daemonxiao <735462752@qq.com>
Co-authored-by: iosmanthus <myosmanthustree@gmail.com>
@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. You are not a reviewer or committer or co-leader or leader.

@iosmanthus iosmanthus merged commit 75f0586 into tikv:master Jun 14, 2022
ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Jun 14, 2022
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Collaborator

cherry pick to release-3.3 in PR #612

iosmanthus pushed a commit that referenced this pull request Jun 14, 2022
Co-authored-by: Daemonxiao <35677990+Daemonxiao@users.noreply.github.com>
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