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

Fix the time check logic for judging stale client channels to be inac… #18340

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

xiaohu-liu
Copy link

What changes are proposed in this pull request?

Fix bug involved by #18332.
Alter the time judgment logic for judging whether stale client channels are inactive. Using the LocaTime object cannot correctly judge whether a channel client is inactive, because a LocalTime plus or minus time offset only changes the hour, minute, second attribute value, and It will not affect the date, you actually need to use the LocalDateTime object instead.

Why are the changes needed?

Please clarify why the changes are needed. For instance,
In the code, the LocaTime class is used to determine that a client channel is inactive. The LocalTime object adds or subtracts the time offset. It only changes the hour, minute and second attribute value and does not affect the date. In fact, you need to use the LocalDateTime object. In other words, the three-day certification cycle judgment should be based on date and time, not just time.

Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
None

@alluxio-bot
Copy link
Contributor

Thank you for your pull request.
In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement (CLA).
It's all electronic and will take just a few minutes. Please download CLA form here, sign, and e-mail back to cla@alluxio.org

@YichuanSun
Copy link
Contributor

Thanks for your fix, would you mind sending us the CLA like alluxio-bot said? @xiaohu-liu

Copy link
Contributor

@YichuanSun YichuanSun left a comment

Choose a reason for hiding this comment

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

I have read the related issue, remaining a question: is the remove of Inactive channels every three days in your expectation or not?

@xiaohu-liu
Copy link
Author

xiaohu-liu commented Oct 30, 2023

The default value actually meets our needs. What is the default value of 3 days based on? @YichuanSun

@YichuanSun
Copy link
Contributor

The default value actually meets our needs. What is the default value of 3 days based on? @YichuanSun

I don't know either, it is just like a magic number.

@YichuanSun YichuanSun added the type-bug This issue is about a bug label Oct 30, 2023
@xiaohu-liu
Copy link
Author

xiaohu-liu commented Oct 30, 2023

Thanks for your fix, would you mind sending us the CLA like alluxio-bot said? @xiaohu-liu

is there any tool to generate an electronic signature ? what type of tool you advice to use?

@YichuanSun
Copy link
Contributor

YichuanSun commented Oct 30, 2023

I think it can be merged if you have sent the CLA. Thanks for your work! @xiaohu-liu

@YichuanSun
Copy link
Contributor

Aha, the first time I wrote my name with an ink pen, took a picture, then used a scanning app to generate a black-white pdf file, then cut my electronic signature and pasted it on the CLA pdf.

@YichuanSun
Copy link
Contributor

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit 660e00d into Alluxio:main Oct 31, 2023
12 checks passed
@xiaohu-liu xiaohu-liu deleted the f-fix-inactive-check branch October 31, 2023 02:39
ssz1997 pushed a commit to ssz1997/alluxio that referenced this pull request Dec 15, 2023
### What changes are proposed in this pull request?

Fix bug involved by Alluxio#18332.
Alter the time judgment logic for judging whether stale client channels are inactive. Using the LocaTime object cannot correctly judge whether a channel client is inactive, because a LocalTime plus or minus time offset only changes the hour, minute, second attribute value, and It will not affect the date, you actually need to use the LocalDateTime object instead.
### Why are the changes needed?

Please clarify why the changes are needed. For instance,
In the code, the LocaTime class is used to determine that a client channel is inactive. The LocalTime object adds or subtracts the time offset. It only changes the hour, minute and second attribute value and does not affect the date. In fact, you need to use the LocalDateTime object.  In other words, the three-day certification cycle judgment should be based on date and time, not just time.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
None

			pr-link: Alluxio#18340
			change-id: cid-5b69e0c87d3bad8556ae27d491f3e0dc567378b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug This issue is about a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants