lighting: expose limited fields for `kv.Session` #55517

lcwangchao · 2024-08-20T04:36:52Z

What problem does this PR solve?

Issue Number: ref #53388

What changed and how does it work?

This PR makes some changes to kv.Session to avoid expose all fields of sessionctx.Context. This approach can make developers more clear about what contexts are used in lightning and easier to check whether the mocked session have been constructed properly.

This PR makes the old implementation of sessionctx.Context private, and in the further PRs, we'll remove this private session struct.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

tiprow · 2024-08-20T04:37:13Z

Hi @lcwangchao. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

lcwangchao · 2024-08-20T04:39:00Z

pkg/lightning/backend/kv/session_test.go

-)
-
-func TestSession(t *testing.T) {
-	session := kv.NewSessionCtx(&encode.SessionOptions{SQLMode: mysql.ModeNone, Timestamp: 1234567890}, log.L())


This test is deleted because kv.NewSessionCtx is deleted

lcwangchao · 2024-08-20T04:39:45Z

pkg/lightning/backend/tidb/tidb.go

 	return &tidbEncoder{
 		mode:   config.SQLMode,
 		tbl:    config.Table,
-		se:     se,


seems tidbEncoder.se is useless, just delete it.

codecov · 2024-08-20T06:02:03Z

Codecov Report

Attention: Patch coverage is 88.42105% with 11 lines in your changes missing coverage. Please review.

Project coverage is 74.8227%. Comparing base (509d1bd) to head (5400604).
Report is 17 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #55517        +/-   ##
================================================
- Coverage   74.8899%   74.8227%   -0.0673%     
================================================
  Files          1576       1598        +22     
  Lines        365726     452241     +86515     
================================================
+ Hits         273892     338379     +64487     
- Misses        72057      92961     +20904     
- Partials      19777      20901      +1124

Flag	Coverage Δ
integration	`50.7597% <82.1052%> (?)`
unit	`71.9746% <75.2688%> (-1.8865%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9567% <ø> (-2.2327%)`	⬇️
parser	`∅ <ø> (∅)`
br	`51.8558% <ø> (+4.0577%)`	⬆️

lcwangchao · 2024-08-20T06:34:02Z

pkg/lightning/backend/kv/session.go

 // transaction type and provides the session variables to the TiDB library
 // optimized for Lightning.
-type Session struct {
+type session struct {


Here I make the original session private because not all fields in sessionctx.Context will be used. The public
struct Session will wrap this session and provide some necessary methods.

please add this as code comment

D3Hunter

rest lgtm

pkg/lightning/backend/kv/session.go

D3Hunter · 2024-08-20T08:06:48Z

pkg/lightning/backend/kv/session.go

 // transaction type and provides the session variables to the TiDB library
 // optimized for Lightning.
-type Session struct {
+type session struct {


please add this as code comment

ti-chi-bot · 2024-08-20T09:20:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, tangenta

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [D3Hunter,tangenta]
~~lightning/OWNERS~~ [D3Hunter]
~~pkg/executor/importer/OWNERS~~ [D3Hunter]
~~pkg/lightning/OWNERS~~ [D3Hunter]
~~pkg/table/OWNERS~~ [tangenta]

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

ti-chi-bot · 2024-08-20T09:21:02Z

[LGTM Timeline notifier]

Timeline:

2024-08-20 09:16:12.129365635 +0000 UTC m=+256967.263815756: ☑️ agreed by D3Hunter.
2024-08-20 09:21:01.522494436 +0000 UTC m=+257256.656944566: ☑️ agreed by tangenta.

lcwangchao · 2024-08-20T09:42:52Z

/retest

tiprow · 2024-08-20T09:43:14Z

@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

lcwangchao · 2024-08-20T13:13:04Z

/retest

tiprow · 2024-08-20T13:13:26Z

@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

lcwangchao · 2024-08-20T14:18:02Z

/retest

tiprow · 2024-08-20T14:18:24Z

@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

…e-upstream * origin/master: parser: call `SetText` correctly for `CreateViewStmt` (#55520) lighting: expose limited fields for `kv.Session` (#55517) show: prefilter table name with like pattern and show full tables (#55396) Planner: Do not allow cardinality to go below 1 (#55242) *: replace maps.Copy which is for map clone with std maps.Clone (#55530) infoschema: add WithRefillOption for TableByName and TableByID (#55511) domain: change groupSize in splitForConcurrentFetch (#55518) ddl: fix resuming to wrong checkpoint when failed during adding index (#55506)

…e-upstream * origin/master: parser: call `SetText` correctly for `CreateViewStmt` (pingcap#55520) lighting: expose limited fields for `kv.Session` (pingcap#55517) show: prefilter table name with like pattern and show full tables (pingcap#55396) Planner: Do not allow cardinality to go below 1 (pingcap#55242) *: replace maps.Copy which is for map clone with std maps.Clone (pingcap#55530) infoschema: add WithRefillOption for TableByName and TableByID (pingcap#55511) domain: change groupSize in splitForConcurrentFetch (pingcap#55518) ddl: fix resuming to wrong checkpoint when failed during adding index (pingcap#55506) Signed-off-by: Wish <breezewish@outlook.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 20, 2024

lcwangchao commented Aug 20, 2024

View reviewed changes

lcwangchao force-pushed the lightingse branch 3 times, most recently from c6aa732 to 2f874c9 Compare August 20, 2024 05:38

lighting: expose limited fields for kv.Session

b7ff623

lcwangchao force-pushed the lightingse branch from 2f874c9 to b7ff623 Compare August 20, 2024 05:59

lcwangchao requested review from lance6716 and YangKeao August 20, 2024 06:29

lcwangchao commented Aug 20, 2024

View reviewed changes

D3Hunter reviewed Aug 20, 2024

View reviewed changes

lcwangchao force-pushed the lightingse branch from e9420cd to 33e407c Compare August 20, 2024 09:15

D3Hunter approved these changes Aug 20, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Aug 20, 2024

tangenta approved these changes Aug 20, 2024

View reviewed changes

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 20, 2024

adress comments

5400604

lcwangchao force-pushed the lightingse branch from 33e407c to 5400604 Compare August 20, 2024 13:19

ti-chi-bot bot merged commit 1f095a3 into pingcap:master Aug 20, 2024
22 checks passed

lcwangchao deleted the lightingse branch August 20, 2024 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lighting: expose limited fields for `kv.Session` #55517

lighting: expose limited fields for `kv.Session` #55517

lcwangchao commented Aug 20, 2024

tiprow bot commented Aug 20, 2024

lcwangchao Aug 20, 2024

lcwangchao Aug 20, 2024

codecov bot commented Aug 20, 2024 •

edited

Loading

lcwangchao Aug 20, 2024

D3Hunter Aug 20, 2024

D3Hunter left a comment

D3Hunter Aug 20, 2024

ti-chi-bot bot commented Aug 20, 2024

ti-chi-bot bot commented Aug 20, 2024

lcwangchao commented Aug 20, 2024

tiprow bot commented Aug 20, 2024

lcwangchao commented Aug 20, 2024

tiprow bot commented Aug 20, 2024

lcwangchao commented Aug 20, 2024

tiprow bot commented Aug 20, 2024

lighting: expose limited fields for kv.Session #55517

lighting: expose limited fields for kv.Session #55517

Conversation

lcwangchao commented Aug 20, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Aug 20, 2024

lcwangchao Aug 20, 2024

Choose a reason for hiding this comment

lcwangchao Aug 20, 2024

Choose a reason for hiding this comment

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

lcwangchao Aug 20, 2024

Choose a reason for hiding this comment

D3Hunter Aug 20, 2024

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

D3Hunter Aug 20, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 20, 2024

ti-chi-bot bot commented Aug 20, 2024

[LGTM Timeline notifier]

lcwangchao commented Aug 20, 2024

tiprow bot commented Aug 20, 2024

lcwangchao commented Aug 20, 2024

tiprow bot commented Aug 20, 2024

lcwangchao commented Aug 20, 2024

tiprow bot commented Aug 20, 2024

lighting: expose limited fields for `kv.Session` #55517

lighting: expose limited fields for `kv.Session` #55517

codecov bot commented Aug 20, 2024 •

edited

Loading