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

refactor session part and statement part to improve code readability #3482

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

googs1025
Copy link
Member

@googs1025 googs1025 commented May 23, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

  • refactor session part to improve readability. We can reduce if-else branches and reduce code complexity
  • refactor statement part

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

Signed-off-by: googs1025 <googs1025@gmail.com>
@volcano-sh-bot volcano-sh-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 23, 2024
@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 23, 2024
@googs1025
Copy link
Member Author

/cc @lowang-bh @hwdef @Monokaix

Copy link
Member

@lowang-bh lowang-bh left a comment

Choose a reason for hiding this comment

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

/lgtm

@volcano-sh-bot volcano-sh-bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels May 23, 2024
@volcano-sh-bot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign lowang-bh
You can assign the PR to them by writing /assign @lowang-bh in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: googs1025 <googs1025@gmail.com>
@lowang-bh
Copy link
Member

/hold

@volcano-sh-bot volcano-sh-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 24, 2024
@googs1025 googs1025 changed the title refactor session part to improve code readability refactor session part and statement part to improve code readability May 26, 2024
@googs1025 googs1025 requested a review from lowang-bh May 28, 2024 08:07
@Monokaix
Copy link
Member

/hold
Any consideration here?

@lowang-bh
Copy link
Member

Session refact is ok. The statement refact has some information lose. The log is important and should be easy to debug issue in production env.

@googs1025
Copy link
Member Author

Session refact is ok. The statement refact has some information lose. The log is important and should be easy to debug issue in production env.

I have kept most of the logs.
can you help me to review it, and point it out to me if I missed anything and I will modify it immediately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants