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.

Sign up for GitHub

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

Jump to bottom

[close #446] Fix Optional fields type #445

Merged
merged 1 commit into from
Dec 27, 2021
Merged

[close #446] Fix Optional fields type #445

merged 1 commit into from
Dec 27, 2021

Conversation

zhongqishang
Copy link
Contributor

@zhongqishang zhongqishang commented Dec 24, 2021 •
edited

Signed-off-by: Qishang Zhong zhongqishang@gmail.com

What problem does this PR solve?

When I use the client in Flink , I get an error

Caused by: java.io.NotSerializableException: java.util.Optional
  at java.base/java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1185)
  at java.base/java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:349)
  at org.apache.flink.util.InstantiationUtil.serializeObject(InstantiationUtil.java:624)
  at org.apache.flink.api.java.ClosureCleaner.clean(ClosureCleaner.java:143)
  ... 48 more

What is changed and how it works?

Optional in the getter method

Check List for Tests

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

  • Unit test

Side effects

  • NO side effects

Related changes

  • NO related changes

https://stackoverflow.com/questions/24547673/why-java-util-optional-is-not-serializable-how-to-serialize-the-object-with-suc

marsishandsome
marsishandsome approved these changes Dec 27, 2021
View reviewed changes
Copy link
Collaborator

@marsishandsome marsishandsome left a comment

Choose a reason for hiding this comment

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

LGTM

@marsishandsome
Copy link
Collaborator

/run-all-tests

@zz-jason
Copy link
Member

@zhongqishang thanks for your contribution, please create an issue that you wish to fix and follow the PR title format

@zz-jason
Copy link
Member

could you also add some tests?

@zhongqishang zhongqishang changed the title Fix Optional fields type [close #446] Fix Optional fields type Dec 27, 2021
@zhongqishang
Copy link
Contributor Author

@zz-jason I have created an issue and added the tests.

@zhongqishang
Copy link
Contributor Author

/run-all-tests

@zhongqishang
Fix fields type
45bc81c 
Signed-off-by: Qishang Zhong <zhongqishang@gmail.com>
zz-jason
zz-jason reviewed Dec 27, 2021
View reviewed changes
src/main/java/org/tikv/common/TiConfiguration.java
getIntOption(TIKV_RAWKV_BATCH_READ_SLOWLOG_IN_MS);
private Optional<Integer> rawKVBatchWriteSlowLogInMS =
getIntOption(TIKV_RAWKV_BATCH_WRITE_SLOWLOG_IN_MS);
private Integer rawKVReadSlowLogInMS = getIntOption(TIKV_RAWKV_READ_SLOWLOG_IN_MS).orElse(null);
Copy link
Member

@zz-jason zz-jason Dec 27, 2021 •
edited

Choose a reason for hiding this comment

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

I'm a little confused. why not set the variable type of rawKVReadSlowLogInMS to int and get the value via getInt() just like what we did for rawKVCleanTimeoutInMS?

@marsishandsome do you have any idea?

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 rawKVReadSlowLogInMS not set default value in org.tikv.common.TiConfiguration#loadFromDefaultProperties, and rawKVReadSlowLogInMS must be have a default value due to org.tikv.common.log.SlowLogImpl#log.

Copy link
Collaborator

Choose a reason for hiding this comment

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

rawKVReadSlowLogInMS is calculated as follows:

  1. if TIKV_RAWKV_READ_SLOWLOG_IN_MS is set, then use it, in such case rawKVReadSlowLogInMS != null
  2. else use getTimeout() * 2, in such case rawKVReadSlowLogInMS == null

Copy link
Member

Choose a reason for hiding this comment

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

OK.

zz-jason
zz-jason approved these changes Dec 27, 2021
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

/merge

@ti-srebot
Copy link
Collaborator

/run-all-tests

1 similar comment
@zz-jason
Copy link
Member

/run-all-tests

@ti-srebot
Copy link
Collaborator

@zhongqishang merge failed.

@zz-jason zz-jason merged commit b94b3cb into tikv:master Dec 27, 2021
ti-srebot pushed a commit to ti-srebot/client-java that referenced this pull request Dec 27, 2021
@zhongqishang @ti-srebot
cherry pick tikv#445 to release-3.1
ba18c5e 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Dec 27, 2021
Closed
@ti-srebot
Copy link
Collaborator

cherry pick to release-3.1 in PR #449

@zhongqishang zhongqishang mentioned this pull request Dec 28, 2021
Merged
zz-jason pushed a commit that referenced this pull request Dec 29, 2021
@zhongqishang
[close #446] Fix Optional fields type (#445) (#451)
48f0306
@marsishandsome marsishandsome mentioned this pull request Feb 11, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marsishandsome marsishandsome marsishandsome approved these changes

@zz-jason zz-jason zz-jason approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
component/common status/can-merge status/LGT2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zhongqishang @marsishandsome @zz-jason @ti-srebot