Skip to content

git/githistory/log: teach Durable() to Task#2334

Merged
ttaylorr merged 8 commits intomasterfrom
githistory-log-durable-tasks
Jun 19, 2017
Merged

git/githistory/log: teach Durable() to Task#2334
ttaylorr merged 8 commits intomasterfrom
githistory-log-durable-tasks

Conversation

@ttaylorr
Copy link
Contributor

This pull request teaches a new Task type to the github.com/git-lfs/git-lfs/git/githistory/log package: DurableTask.

A DurableTask ensures that all log <-Update() reads are logged to the sink io.Writer, regardless of throttling conditions. This required for multi-line log updates (like output from git-ref-update(1)) where all log lines are important.


/cc @git-lfs/core
/refs #2146 #2329

@technoweenie
Copy link
Contributor

Why is this needed? If the tasks need to be durable, then they probably don't need to be throttled in the first place. Do only some of the tasks need this?

@ttaylorr
Copy link
Contributor Author

ttaylorr commented Jun 19, 2017

Why is this needed? If the tasks need to be durable, then they probably don't need to be throttled in the first place. Do only some of the tasks need this?

Good question 😄 Only some of the tasks need this. For instance, the *PercentageTask writes updates every time Count() is called, and usually gets throttled. The *ListTask type (see: #2335) yields updates at an unknown rate, but it's important that all updates are logged. In other words, ListTask satisfies DurableTask, but *PercentageTask does not.

@technoweenie
Copy link
Contributor

Ah, that makes sense after looking at that PR. I would just add Durable() to the base Task interface, since there are only a few task types. What do you think about Throttled() for a name instead of Durable()?

@ttaylorr
Copy link
Contributor Author

Ah, that makes sense after looking at that PR.

Thanks for taking a look, and sorry for not explaining things more thoroughly in this PR.

I would just add Durable() to the base Task interface, since there are only a few task types.

Good call, I like the explicitness of being able to declare that a task is Durable() or not per task. I added that in a7a99e5, 8c64a43 & 5223586.

What do you think about Throttled() for a name instead of Durable()?

I think I prefer the Durable() name, since Throttled() feels like a negated conditional.

@technoweenie
Copy link
Contributor

I just don't think Durable accurately describes what is happening. Maybe I'm just used to seeing that word in the context of a data store.

Copy link
Contributor

@technoweenie technoweenie left a comment

Choose a reason for hiding this comment

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

Still don't like Durable(), as it's not losing data.

Another idea is something like RefreshRate() time.Duration, so tasks can specify their own update frequency.

That said, keep on rocking with Durable if you like.

@ttaylorr ttaylorr changed the base branch from migrate-logger to master June 19, 2017 21:16
@ttaylorr ttaylorr merged commit 99c0f45 into master Jun 19, 2017
@ttaylorr ttaylorr deleted the githistory-log-durable-tasks branch June 19, 2017 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants