~rockorager/offmap

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
8 4

[PATCH offmap 1/2] cmd: rename sync function to avoid collisions with stdlib package

Moritz Poldrack <git@moritz.sh>
Details
Message ID
<20221214194705.394747-1-git@moritz.sh>
DKIM signature
missing
Download raw message
Patch: +2 -2
Signed-off-by: Moritz Poldrack <git@moritz.sh>
---
 cmd/sync.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmd/sync.go b/cmd/sync.go
index 328e841..353d94f 100644
--- a/cmd/sync.go
+++ b/cmd/sync.go
@@ -15,13 +15,13 @@ func newSyncCommand() *cobra.Command {
	cmd := &cobra.Command{
		Use:   "sync",
		Short: "sync synchronizes mail stores",
		Run:   sync,
		Run:   syncRun,
	}
	cmd.Flags().StringSliceP("account", "a", nil, "only sync the specified account(s)")
	return cmd
}

func sync(cmd *cobra.Command, args []string) {
func syncRun(cmd *cobra.Command, args []string) {
	verbosity, _ := cmd.Flags().GetCount("verbose")
	log.SetLevel(verbosity)
	accts, err := cmd.Flags().GetStringSlice("account")
-- 
2.39.0

[PATCH offmap 2/2] cmd: make syncing run in parallel

Moritz Poldrack <git@moritz.sh>
Details
Message ID
<20221214194705.394747-2-git@moritz.sh>
In-Reply-To
<20221214194705.394747-1-git@moritz.sh> (view parent)
DKIM signature
missing
Download raw message
Patch: +84 -76
Signed-off-by: Moritz Poldrack <git@moritz.sh>
---
 cmd/sync.go | 160 +++++++++++++++++++++++++++-------------------------
 1 file changed, 84 insertions(+), 76 deletions(-)

diff --git a/cmd/sync.go b/cmd/sync.go
index 353d94f..fbaa48c 100644
--- a/cmd/sync.go
+++ b/cmd/sync.go
@@ -1,6 +1,7 @@
package main

import (
	"sync"
	"sync/atomic"
	"time"

@@ -33,96 +34,103 @@ func syncRun(cmd *cobra.Command, args []string) {
	if err != nil {
		log.Debugf("%v", err)
	}
	var wg sync.WaitGroup
	for _, cfg := range cfgs {
		log.Infof("offmap: syncing account %s", cfg.Name)
		start := time.Now()
		wg.Add(1)
		go func(cfg *offmap.Config) {
			defer wg.Done()
			log.Infof("offmap: syncing account %s", cfg.Name)
			start := time.Now()

		// Bootstrap the app
		state, err := offmap.NewState(cfg)
		if err != nil {
			log.Fatalf("%v", err)
		}
		err = state.Load()
		if err != nil {
			log.Fatalf("%v", err)
		}
		local, err := maildir.NewStore(cfg)
		if err != nil {
			log.Fatalf("could not create store: %v", err)
		}
		remote, err := imap.NewStore(cfg)
		if err != nil {
			log.Fatalf("could not create store: %v", err)
		}

		// Get the diffs
		localDiff, err := local.Diff(state.Mailboxes)
		if err != nil {
			log.Fatalf("%v", err)
		}
		remoteDiff, err := remote.Diff(state.Mailboxes)
		if err != nil {
			log.Fatalf("%v", err)
		}

		var quit atomic.Bool
		go func() {
			// Save our state if we cancel the command
			<-cmd.Context().Done()
			quit.Store(true)
		}()

		// Do mailboxes first
		err = remote.ApplyMailboxDiff(state, localDiff)
		if err != nil {
			// Save any changes we've made. log.Fatalf calls os.Exit(1).
			// Defers are bypassed with this call
			if err := state.Save(); err != nil {
				log.Errorf("offmap: could not save state: %v", err)
			// Bootstrap the app
			state, err := offmap.NewState(cfg)
			if err != nil {
				log.Fatalf("%v", err)
			}
			log.Fatalf("imap: could not apply diff: %v", err)
		}

		// Apply remote changes to local
		err = local.ApplyMailboxDiff(state, remoteDiff)
		if err != nil {
			// Save any changes we've made. log.Fatalf calls os.Exit(1).
			// Defers are bypassed with this call
			if err := state.Save(); err != nil {
				log.Errorf("offmap: could not save state: %v", err)
			err = state.Load()
			if err != nil {
				log.Fatalf("%v", err)
			}
			local, err := maildir.NewStore(cfg)
			if err != nil {
				log.Fatalf("could not create store: %v", err)
			}
			remote, err := imap.NewStore(cfg)
			if err != nil {
				log.Fatalf("could not create store: %v", err)
			}
			log.Fatalf("%v", err)
		}

		// Now the state has an up to date picture of mailboxes. Sync
		// each mailbox, if it has changes
		for _, mbox := range state.Mailboxes {
			if quit.Load() {
				break
			// Get the diffs
			localDiff, err := local.Diff(state.Mailboxes)
			if err != nil {
				log.Fatalf("%v", err)
			}
			remoteDiff, err := remote.Diff(state.Mailboxes)
			if err != nil {
				log.Fatalf("%v", err)
			}
			ok1, err := remote.ApplyEmailDiff(state, mbox.RemoteName, localDiff)

			var quit atomic.Bool
			go func() {
				// Save our state if we cancel the command
				<-cmd.Context().Done()
				quit.Store(true)
			}()

			// Do mailboxes first
			err = remote.ApplyMailboxDiff(state, localDiff)
			if err != nil {
				log.Errorf("%v", err)
				continue
				// Save any changes we've made. log.Fatalf calls os.Exit(1).
				// Defers are bypassed with this call
				if err := state.Save(); err != nil {
					log.Errorf("offmap: could not save state: %v", err)
				}
				log.Fatalf("imap: could not apply diff: %v", err)
			}
			ok2, err := local.ApplyEmailDiff(state, mbox.LocalName, remoteDiff)

			// Apply remote changes to local
			err = local.ApplyMailboxDiff(state, remoteDiff)
			if err != nil {
				log.Errorf("%v", err)
				continue
				// Save any changes we've made. log.Fatalf calls os.Exit(1).
				// Defers are bypassed with this call
				if err := state.Save(); err != nil {
					log.Errorf("offmap: could not save state: %v", err)
				}
				log.Fatalf("%v", err)
			}
			if ok1 || ok2 {
				err = remote.UpdateMailboxStatus(state, mbox.LocalName)

			// Now the state has an up to date picture of mailboxes. Sync
			// each mailbox, if it has changes
			for _, mbox := range state.Mailboxes {
				if quit.Load() {
					break
				}
				ok1, err := remote.ApplyEmailDiff(state, mbox.RemoteName, localDiff)
				if err != nil {
					log.Errorf("%v", err)
					continue
				}
				ok2, err := local.ApplyEmailDiff(state, mbox.LocalName, remoteDiff)
				if err != nil {
					log.Errorf("%v", err)
					continue
				}
				if ok1 || ok2 {
					err = remote.UpdateMailboxStatus(state, mbox.LocalName)
					if err != nil {
						log.Errorf("%v", err)
					}
				}
			}
		}

		// Save and clean up
		if err := state.Save(); err != nil {
			log.Errorf("offmap: could not save state: %v", err)
		}
		remote.Cleanup()
		log.Infof("offmap (%s): sync completed in %s", cfg.Name, time.Since(start).Round(1*time.Millisecond))
			// Save and clean up
			if err := state.Save(); err != nil {
				log.Errorf("offmap: could not save state: %v", err)
			}
			remote.Cleanup()
			log.Infof("offmap (%s): sync completed in %s", cfg.Name, time.Since(start).Round(1*time.Millisecond))
		}(cfg)
	}

	wg.Wait()
}
-- 
2.39.0

[offmap/patches/.build.yml] build success

builds.sr.ht <builds@sr.ht>
Details
Message ID
<CP1SUH0TN438.35UEP08HX2124@cirno2>
In-Reply-To
<20221214194705.394747-2-git@moritz.sh> (view parent)
DKIM signature
missing
Download raw message
offmap/patches/.build.yml: SUCCESS in 1m54s

[cmd: rename sync function to avoid collisions with stdlib package][0] from [Moritz Poldrack][1]

[0]: https://lists.sr.ht/~rockorager/offmap/patches/37529
[1]: git@moritz.sh

✓ #905181 SUCCESS offmap/patches/.build.yml https://builds.sr.ht/~rockorager/job/905181
Details
Message ID
<CP2KROA7Z73G.4ITFVNG9ZNOD@spunky>
In-Reply-To
<20221214194705.394747-1-git@moritz.sh> (view parent)
DKIM signature
missing
Download raw message
On Wed Dec 14, 2022 at 1:47 PM CST, Moritz Poldrack wrote:
> Signed-off-by: Moritz Poldrack <git@moritz.sh>
> ---

How about "synchronize"?

Re: [PATCH offmap 2/2] cmd: make syncing run in parallel

Details
Message ID
<CP2KU5NFZPA3.109RV8OACPWTK@spunky>
In-Reply-To
<20221214194705.394747-2-git@moritz.sh> (view parent)
DKIM signature
missing
Download raw message
On Wed Dec 14, 2022 at 1:47 PM CST, Moritz Poldrack wrote:
> Signed-off-by: Moritz Poldrack <git@moritz.sh>
> ---

I like it! The one comment I have is we should change all of the log.Fatalf's in
the account goroutines into log.Errorf's + return. That way one account sync can
fail and not pull down the entire program. What do you think?

--
Tim

Re: [PATCH offmap 2/2] cmd: make syncing run in parallel

Details
Message ID
<CP2NGZIJ2I8Z.1L2AY520UVVIN@hades.moritz.sh>
In-Reply-To
<CP2KU5NFZPA3.109RV8OACPWTK@spunky> (view parent)
DKIM signature
missing
Download raw message
On Thu Dec 15, 2022 at 6:45 PM CET, Tim Culverhouse wrote:
> On Wed Dec 14, 2022 at 1:47 PM CST, Moritz Poldrack wrote:
> > Signed-off-by: Moritz Poldrack <git@moritz.sh>
> > ---
>
> I like it! The one comment I have is we should change all of the log.Fatalf's in
> the account goroutines into log.Errorf's + return. That way one account sync can
> fail and not pull down the entire program. What do you think?
I think the os.Exit(1) in log.Fatal should be removed completely. Right
now a log.Fatal is basically worse than a panic since defers are not
run. So I think it should be part of another patchseries to make
cancellations and abortions (is that the correct word? sounds strange in
this context) less of a nuclear options and have them clean up behind
them (move already downloaded messages to cur, logout for all
connections, potentially other cleanup tasks…)

-- 
Moritz Poldrack
https://moritz.sh
Details
Message ID
<CP2NJUFIMY4W.2D3AWOHJMHZ1T@hades.moritz.sh>
In-Reply-To
<CP2KROA7Z73G.4ITFVNG9ZNOD@spunky> (view parent)
DKIM signature
missing
Download raw message
On Thu Dec 15, 2022 at 6:41 PM CET, Tim Culverhouse wrote:
> On Wed Dec 14, 2022 at 1:47 PM CST, Moritz Poldrack wrote:
> > Signed-off-by: Moritz Poldrack <git@moritz.sh>
> > ---
>
> How about "synchronize"?
I actually quite like having the command name in the function name.

-- 
Moritz Poldrack
https://moritz.sh

Re: [PATCH offmap 2/2] cmd: make syncing run in parallel

Details
Message ID
<CP2OT3YWZXDC.ZNXG9NMOVOGP@spunky>
In-Reply-To
<CP2NGZIJ2I8Z.1L2AY520UVVIN@hades.moritz.sh> (view parent)
DKIM signature
missing
Download raw message
On Thu Dec 15, 2022 at 1:49 PM CST, Moritz Poldrack wrote:
> On Thu Dec 15, 2022 at 6:45 PM CET, Tim Culverhouse wrote:
> > On Wed Dec 14, 2022 at 1:47 PM CST, Moritz Poldrack wrote:
> > > Signed-off-by: Moritz Poldrack <git@moritz.sh>
> > > ---
> >
> > I like it! The one comment I have is we should change all of the log.Fatalf's in
> > the account goroutines into log.Errorf's + return. That way one account sync can
> > fail and not pull down the entire program. What do you think?
> I think the os.Exit(1) in log.Fatal should be removed completely. Right
> now a log.Fatal is basically worse than a panic since defers are not
> run. So I think it should be part of another patchseries to make
> cancellations and abortions (is that the correct word? sounds strange in
> this context) less of a nuclear options and have them clean up behind
> them (move already downloaded messages to cur, logout for all
> connections, potentially other cleanup tasks…)

I agree - I actually (had) one that changed the calls to sync and diff to the
RunE method of cobra, and returned errors back up to the main loop, but that
becomes a moot point with this series to just return and exit the goroutine. We
can do that all as a separate patchset.

I'm fine with keeping syncRun in (1/2) too.

Reviewed-by: Tim Culverhouse <tim@timculverhouse.com>
Details
Message ID
<CP41T086HSDA.2SLJ4N7MW9YQM@hades.moritz.sh>
In-Reply-To
<20221214194705.394747-1-git@moritz.sh> (view parent)
DKIM signature
missing
Download raw message
Applied.

To ssh://git@git.sr.ht/~rockorager/offmap
   faea313..f702f57  main -> main
-- 
Moritz Poldrack
https://moritz.sh
Reply to thread Export thread (mbox)