lightning: remote backend #58789

zeminzhou · 2025-01-08T06:17:02Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What changed and how does it work?

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

Signed-off-by: zeminzhou <[email protected]>

tiprow · 2025-01-08T06:17:19Z

Hi @zeminzhou. 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.

Signed-off-by: zeminzhou <[email protected]>

codecov · 2025-01-08T06:45:54Z

Codecov Report

Attention: Patch coverage is 25.58140% with 832 lines in your changes missing coverage. Please review.

Project coverage is 73.5291%. Comparing base (c199ddf) to head (cd3d941).
Report is 101 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58789        +/-   ##
================================================
+ Coverage   73.0885%   73.5291%   +0.4405%     
================================================
  Files          1676       1721        +45     
  Lines        463643     493818     +30175     
================================================
+ Hits         338870     363100     +24230     
- Misses       103924     108348      +4424     
- Partials      20849      22370      +1521

Flag	Coverage Δ
integration	`45.4801% <5.4561%> (?)`
unit	`72.1440% <23.2758%> (-0.1423%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`42.5734% <ø> (-3.1596%)`	⬇️

ystaticy · 2025-01-08T08:12:06Z

Please add UT for remote backend.

pkg/lightning/backend/remote/chunk_sender.go

pkg/lightning/backend/remote/util.go

pkg/lightning/backend/remote/chunk_sender.go

pkg/lightning/config/config.go

pkg/lightning/config/global.go

lightning/pkg/importer/table_import.go

ystaticy · 2025-01-13T07:07:25Z

lightning/pkg/importer/import.go

@@ -433,13 +434,31 @@ func NewImportControllerWithPauser(
 		if err != nil {
 			return nil, err
 		}
+	case config.BackendRemote:


What error will occur if the user configures the remote backend? Should we prompt the user that this feature is not yet implemented? Additionally, should we add a description in the code comments to avoid confusion for users and developers?

Signed-off-by: zeminzhou <[email protected]>

ti-chi-bot · 2025-01-14T10:31:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign gmhdbjd for approval. For more information see the Code Review Process.

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: zeminzhou <[email protected]>

lance6716 · 2025-01-21T06:14:25Z

/retest

tiprow · 2025-01-21T06:14:47Z

@lance6716: 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.

lance6716

will review later

lightning/pkg/importer/get_pre_info.go

lightning/pkg/importer/import.go

lance6716 · 2025-01-21T06:45:18Z

lightning/pkg/importer/import.go

+		if err != nil {
+			return nil, errors.Trace(err)
+		}
+		pdHTTPCli = pdhttp.NewClientWithServiceDiscovery(


we can add common function to create PD clients. In future we may need to change the construct arguments and forget to modify both backend

lance6716 · 2025-01-21T06:48:09Z

lightning/pkg/importer/import.go

+			pdhttp.WithTLSConfig(tls.TLSConfig()),
+		).WithBackoffer(retry.InitialBackoffer(time.Second, time.Second, pdutil.PDRequestRetryTime*time.Second))
+
+		encodingBuilder = local.NewEncodingBuilder(ctx)


This is the branch of remote backend, but it calls the local package. It makes me think that one nature of local backend is KV encoding in lightning comparing with tidb backend. remote backend also need the local KV encoding functionality, but it has a different way to manage engines. Maybe we can reuse local backend and only add a new type of engines. I'll check this idea when reviewing rest parts

pkg/lightning/backend/remote/chunk.go

pkg/lightning/backend/remote/chunk_sender.go

Signed-off-by: zeminzhou <[email protected]>

ti-chi-bot · 2025-01-22T08:49:11Z

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

_{📖 For more info, you can check the "Contribute Code" section in the development guide.}

Notice: To remove the do-not-merge/needs-tests-checked label, please finished the tests then check the finished items in description.

For example:

Tests

Unit test

Integration test

Manual test (add detailed scripts or steps below)

No code

_{📖 For more info, you can check the "Contribute Code" section in the development guide.}

lance6716

Better to add unit test to reveal the problems first when addressing comments

lance6716 · 2025-01-23T05:36:51Z

pkg/lightning/backend/remote/chunk.go

+	"github.com/pingcap/errors"
+)
+
+type chunkMeta struct {


maybe call it chunk? the name chunkMeta makes me think it only contains metadata, but in fact it contains chunkData

lance6716 · 2025-01-23T05:40:04Z

pkg/lightning/backend/remote/chunk.go

+	usingMem bool
+}
+
+func newChunkCache(loadDataTaskID string, writerID uint64, basePath string, usingMem bool) (*chunkCache, error) {


I see usingMem is decided by configuration. Can you explain how to choose the value when generating configuration? Like if source data size is less than 1GB we can use memory

lance6716 · 2025-01-23T05:43:46Z

pkg/lightning/backend/remote/chunk.go

+		return meta.chunkData, nil
+	}
+
+	path := filepath.Join(c.baseDir, fmt.Sprintf("chunk-%d", chunkID))


create a helper function to encode the file path. It can be used by get, put and clean

lance6716 · 2025-01-23T05:45:35Z

pkg/lightning/backend/remote/chunk.go

+
+func (c *chunkCache) put(chunkID uint64, buf []byte) error {
+	if c.usingMem {
+		c.chunks[chunkID] = chunkMeta{size: len(buf), chunkData: buf}


This is not consistent. You also saved the size in put, but it's not used in get

lance6716 · 2025-01-23T05:47:36Z

pkg/lightning/backend/remote/chunk.go

+	if _, ok := c.chunks[chunkID]; !ok {
+		return nil
+	}
+	delete(c.chunks, chunkID)


can directly delete without checking existence

lance6716 · 2025-01-23T06:01:21Z

pkg/lightning/backend/remote/chunk_sender.go

+	}
+}
+
+func (c *chunkSender) putEmptyChunk(ctx context.Context) error {


what's the purpose of empty chunk? Please add comments

lance6716 · 2025-01-23T06:05:28Z

pkg/lightning/backend/remote/chunk_sender.go

+	state := c.state.Load()
+	lastFlushedChunkID := state.FlushedChunkID + 1
+	for lastFlushedChunkID <= result.FlushedChunkID {
+		err := c.chunksCache.clean(lastFlushedChunkID)


WIll the remote worker restarts again after we clean the chunk? Please add comments to explain the communication protocol and guarantees

lance6716 · 2025-01-23T06:06:29Z

pkg/lightning/backend/remote/chunk_sender.go

+			err = json.Unmarshal(data, result)
+			if err != nil {
+				return errors.Trace(err)
+			}


should check result.Cancled

lance6716 · 2025-01-23T06:07:12Z

pkg/lightning/backend/remote/chunk_sender.go

+		}
+	}
+
+	state := c.state.Load()


lance6716 · 2025-01-23T06:13:17Z

pkg/lightning/backend/remote/chunk_sender.go

+		}
+
+		// make sure all chunks are flushed
+		flushedChunkID := result.FlushedChunkIDs[c.id]


why the HTTP server will return a map? Why it returns the result of other writer? Please add comments

lightning: remote backend

940cbaf

Signed-off-by: zeminzhou <[email protected]>

ti-chi-bot bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 8, 2025

lightning: remote backend

022f728

Signed-off-by: zeminzhou <[email protected]>