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: isolate session_id and session states of diff users. #16592

Merged
merged 5 commits into from
Oct 14, 2024

Conversation

youngsofun
Copy link
Member

@youngsofun youngsofun commented Oct 10, 2024

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

By putting session id under user_name when used as a key.

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@youngsofun youngsofun marked this pull request as draft October 10, 2024 15:20
@youngsofun youngsofun marked this pull request as draft October 10, 2024 15:20
@github-actions github-actions bot added the pr-refactor this PR changes the code base without new features or bugfix label Oct 10, 2024
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 11 files at r1, all commit messages.
Reviewable status: 1 of 11 files reviewed, 2 unresolved discussions (waiting on @youngsofun)


src/meta/app/src/principal/client_session_ident.rs line 75 at r1 (raw file):

        Self::parse(&s)
    }
}

There is no need to manually encode the session_id and user_name with a colon. Just treat them as two key segment:

Suggestion:

impl KeyCodec for UserSessionId {
    fn encode_key(&self, b: KeyBuilder) -> KeyBuilder {
        b.push_str(&self.session_id).push_str(&self.user_name)
    }

    fn decode_key(parser: &mut KeyParser) -> Result<Self, KeyError>
    where Self: Sized {
        let s = parser.next_str()?;
        let u = parser.next_str()?;
        Ok(Self{ session_id: s, user_name: u})
    }
}

src/meta/protos/proto/client_session.proto line 26 at r1 (raw file):

  uint64 ver = 100;
  uint64 min_reader_ver = 101;
  string user_name = 2;

A removed field should be reserved to prevent further use with: https://protobuf.dev/programming-guides/proto3/#reserved

@youngsofun
Copy link
Member Author

Just treat them as two key segment
good idea, thank you

A removed field should be reserved to prevent further use with: https://protobuf.dev/programming-guides/proto3/#reserved

this is not used in production for now, can I skip the step and delete it directly?

@drmingdrmer
Copy link
Member

Just treat them as two key segment
good idea, thank you

A removed field should be reserved to prevent further use with: https://protobuf.dev/programming-guides/proto3/#reserved

this is not used in production for now, can I skip the step and delete it directly?

I'd suggest always following the rule if it doesn't create additional work.
Although it's unlikely to happen, there's still an odd that a binary with this field has been deployed somewhere we're unaware of. It's better to be cautious.

@youngsofun youngsofun marked this pull request as ready for review October 13, 2024 15:03
@youngsofun
Copy link
Member Author

@drmingdrmer Your suggestions have all been adopted. please review again.

@youngsofun youngsofun added this pull request to the merge queue Oct 14, 2024
@youngsofun youngsofun removed this pull request from the merge queue due to a manual request Oct 14, 2024
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 11 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @youngsofun)


src/query/service/src/servers/http/v1/session/client_session_manager.rs line 111 at r2 (raw file):

    pub fn state_key(client_session_id: &str, user_name: &str) -> String {
        format!("{client_session_id}:{user_name}")

The client_session_id and user_name should be escaped to prevent any : characters within them from interfering with the key format.

And if this state_key is not used for transport, the return value can be just any Hash type, such as (client_session_id, user_name), which can be used as hash map key.

Code quote:

    pub fn state_key(client_session_id: &str, user_name: &str) -> String {
        format!("{client_session_id}:{user_name}")

@youngsofun
Copy link
Member Author

@drmingdrmer you are right, I will refactor it to return simply (client_session_id, user_name) in next pr coming soon.

@youngsofun youngsofun added this pull request to the merge queue Oct 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 14, 2024
@drmingdrmer drmingdrmer merged commit 1d93b49 into databendlabs:main Oct 14, 2024
75 of 76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-refactor this PR changes the code base without new features or bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: put client-session related objects under user_name
3 participants