Added Parallelism configuration option to lakectl #8283

ItamarYuran · 2024-10-13T12:34:10Z

Closes #8159

Change Description

Added the option to configure the amount of parallelism for lakectl.

See feature request for more backround.

Currently, the configuration nests in a new section "Options"

github-actions · 2024-10-13T12:41:38Z

E2E Test Results - Quickstart

github-actions · 2024-10-13T12:41:56Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed

Thanks! Concise fix :-)

Confused about the precedence of the 2 settings for parallelism, so asking for a change in case I am right about the required precedence.

arielshaqed · 2024-10-13T13:47:24Z

cmd/lakectl/cmd/root.go

-	defaultSyncParallelism = 25
-	defaultSyncPresign     = true
-	defaultNoProgress      = false
+	defaultParallelismConfig = -1


Can we use 0? If not, we need to document what "-1" means.

arielshaqed · 2024-10-13T13:49:20Z

pkg/local/sync.go

+	parallelismToUse := s.cfg.Parallelism
+	if parallelismToUse <= 0 {
+		parallelismToUse = s.cfg.SyncFlags.Parallelism
+	}
+	for i := 0; i < parallelismToUse; i++ {


2 changes, the first is more important:

AFAIU SyncFlags.Parallelism is more specific than the new setting, so if both are specified we should use it and not the new more global setting.

It is enough to call the var "parallelism", it will obviously be used.

Suggested change

parallelismToUse := s.cfg.Parallelism

if parallelismToUse <= 0 {

parallelismToUse = s.cfg.SyncFlags.Parallelism

}

for i := 0; i < parallelismToUse; i++ {

parallelism := s.cfg.SyncFlags.Parallelism

if parallelism <= 0 {

parallelism = s.cfg.Parallelism

}

for i := 0; i < parallelism; i++ {

Thank you!
I understand, problem is, the default for s.cfg.SyncFlags.Parallelism is 25, so unless the user flagged with a 0 or less, the used value will be the default flag and not the configuration.

github-actions · 2024-10-14T14:11:03Z

🎊 PR Preview 665c8e1 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-8283.surge.sh

🕐 Build time: 0.009s

_{🤖 By surge-preview}

N-o-Z

I'm not sure I understand the flag redundancy here.
You can have both a default param value and override it with a config value if exists it's not mutually exclusive.

N-o-Z · 2024-10-14T16:49:28Z

cmd/lakectl/cmd/root.go

+	defaultParallelismConfig = 25
+	defaultSyncParallelism   = 0


I'm not sure why both are needed.
We should:

Use configuration param if provided

Use modified param value if passed as part of command

Fallback to default param value

My thought was, that positive default value should come from one source only. In case both have positive default values how will we determine which one to use? I chose the config val to be positive by default, following Ariel's change request to go from the more general case to specific case.
In any case, this code run according to the priorities:

no flag no config val - uses 25 as config default

no flag with config - uses config

flag with no config - uses the flag,

flag with config - uses flag

Your priorities seem correct. We have a way to determine whether a flag in Cobra was explicitly provided by the user or not (there's at least one place in the code where we use it AFAIR).
You can use the same method to do that and determine whether we need to use the config value or the param value

N-o-Z

@ItamarYuran thanks! I think we're in the right direction!
Added some more comments, if anything is not clear - let's sync to reduce the review cycle.

N-o-Z · 2024-10-15T14:53:55Z

pkg/local/sync.go

@@ -74,7 +74,11 @@ func (s *SyncManager) Sync(rootPath string, remote *uri.URI, changeSet <-chan *C
 	defer s.progressBar.Stop()

 	wg, ctx := errgroup.WithContext(s.ctx)
-	for i := 0; i < s.cfg.SyncFlags.Parallelism; i++ {
+	parallelism := s.cfg.Parallelism


This change is not needed anymore?

N-o-Z · 2024-10-15T14:55:40Z

cmd/lakectl/cmd/root.go


 	presignMode := getPresignMode(cmd, client)
 	return local.SyncFlags{
+		SetParallelism:   setParallelism,


You don't really need SetParallelism anymore.
Note that you have cfg in your context here, so basically you can make the decision (what value to use for the parallelism) here.

N-o-Z · 2024-10-15T14:57:19Z

pkg/local/config.go

@@ -9,6 +9,7 @@ const (
 )

 type SyncFlags struct {
+	SetParallelism   bool


That's not needed anymore, right?

N-o-Z · 2024-10-15T14:58:04Z

cmd/lakectl/cmd/root.go

@@ -574,6 +579,7 @@ func initConfig() {
 	viper.SetDefault("server.retries.min_wait_interval", defaultMinRetryInterval)
 	viper.SetDefault("experimental.local.posix_permissions.enabled", false)
 	viper.SetDefault("local.skip_non_regular_files", false)
+	viper.SetDefault("options.parallelism", defaultParallelism)


You don't need a default for this value since the default is handled in param

ItamarYuran · 2024-10-16T10:52:47Z

@N-o-Z Thank you for your review!
Got rid of all the redundancies. LMK what do you think.

N-o-Z

@ItamarYuran Perfect!
Please note the single comment remained

N-o-Z · 2024-10-22T16:01:12Z

pkg/local/sync.go

+	parallelism := s.cfg.SyncFlags.Parallelism
+
+	for i := 0; i < parallelism; i++ {


Can you please revert this change (redundant assignment)

arielshaqed

Thanks for the changes, and thanks for continuing the review @N-o-Z in the best way! @N-o-Z 's comment remains, of course.

ItamarYuran added 2 commits October 13, 2024 15:05

let us configurate

5185d14

parallelism

41993e0

ItamarYuran linked an issue Oct 13, 2024 that may be closed by this pull request

Allow for the lakectl local --parallelism flag to have persistent default values through env vars/config #8159

Closed

ItamarYuran requested review from arielshaqed and N-o-Z October 13, 2024 12:34

ItamarYuran added area/lakectl Issues related to lakeFS' command line interface (lakectl) include-changelog PR description should be included in next release changelog labels Oct 13, 2024

no linters no splinters

c76c458

arielshaqed requested changes Oct 13, 2024

View reviewed changes

ItamarYuran requested a review from arielshaqed October 14, 2024 14:11

N-o-Z requested changes Oct 14, 2024

View reviewed changes

one default value

56bf6e1

ItamarYuran force-pushed the 8159-allow-for-the-lakectl-local-parallelism-flag-to-have-persistent-default-values-through-env-varsconfig branch from 665c8e1 to 56bf6e1 Compare October 15, 2024 14:21

N-o-Z requested changes Oct 15, 2024

View reviewed changes

N-o-Z reviewed Oct 15, 2024

View reviewed changes

pkg/local/config.go Outdated

@@ -9,6 +9,7 @@ const (

)

type SyncFlags struct {

SetParallelism bool

Copy link

Member

N-o-Z Oct 15, 2024

Choose a reason for hiding this comment

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

That's not needed anymore, right?

N-o-Z reviewed Oct 15, 2024

View reviewed changes

trimmed some lines

da89478

ItamarYuran requested a review from N-o-Z October 22, 2024 09:17

N-o-Z approved these changes Oct 22, 2024

View reviewed changes

arielshaqed approved these changes Oct 23, 2024

View reviewed changes

no redundencies

7b2c5a4

ItamarYuran merged commit 2f4a142 into master Oct 23, 2024
37 checks passed

ItamarYuran deleted the 8159-allow-for-the-lakectl-local-parallelism-flag-to-have-persistent-default-values-through-env-varsconfig branch October 23, 2024 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Parallelism configuration option to lakectl #8283

Added Parallelism configuration option to lakectl #8283

ItamarYuran commented Oct 13, 2024

github-actions bot commented Oct 13, 2024

github-actions bot commented Oct 13, 2024

arielshaqed left a comment

arielshaqed Oct 13, 2024

arielshaqed Oct 13, 2024

ItamarYuran Oct 13, 2024

github-actions bot commented Oct 14, 2024 •

edited

Loading

N-o-Z left a comment

N-o-Z Oct 14, 2024

ItamarYuran Oct 15, 2024

N-o-Z Oct 15, 2024

N-o-Z left a comment

N-o-Z Oct 15, 2024

N-o-Z Oct 15, 2024

N-o-Z Oct 15, 2024

N-o-Z Oct 15, 2024

ItamarYuran commented Oct 16, 2024

N-o-Z left a comment

N-o-Z Oct 22, 2024

arielshaqed left a comment

		parallelism := s.cfg.SyncFlags.Parallelism

		for i := 0; i < parallelism; i++ {

Added Parallelism configuration option to lakectl #8283

Added Parallelism configuration option to lakectl #8283

Conversation

ItamarYuran commented Oct 13, 2024

Change Description

github-actions bot commented Oct 13, 2024

E2E Test Results - Quickstart

github-actions bot commented Oct 13, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 14, 2024 • edited Loading

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItamarYuran commented Oct 16, 2024

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 14, 2024 •

edited

Loading