Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comment out spammy log message during image pull #1061

Closed
wants to merge 1 commit into from

Conversation

dperny
Copy link
Collaborator

@dperny dperny commented Jun 24, 2016

Commented out the spammy log message during image pull. The existing
code seems adequate to report most errors. There may be a more correct
way to report pull status, but I don't know what it is.

Closes #935

Signed-off-by: Drew Erny [email protected]

@dperny
Copy link
Collaborator Author

dperny commented Jun 24, 2016

@stevvooe @aaronlehmann PTAL. Nothing suggests to me that simply commenting out the offending log message isn't a passable solution, but I might be overlooking something.

@dperny
Copy link
Collaborator Author

dperny commented Jun 24, 2016

whoops this is what i get for not building locally one a 1-line change.

Commented out the spammy log message during image pull. The existing
code seems adequate to report most errors. There may be a more correct
way to report pull status, but I don't know what it is.

Closes moby#935

Signed-off-by: Drew Erny <[email protected]>
@dperny dperny force-pushed the 935-obnoxoius-logs-be-gone branch from d89402a to 11d146c Compare June 24, 2016 18:42
@aaronlehmann
Copy link
Collaborator

I think it's okay to remove the pull progress for now, until we have a better way of reporting it.

@codecov-io
Copy link

Current coverage is 53.95%

Merging #1061 into master will decrease coverage by 0.03%

@@             master      #1061   diff @@
==========================================
  Files            75         75          
  Lines         11686      11681     -5   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           6310       6303     -7   
- Misses         4477       4479     +2   
  Partials        899        899          

Sunburst

Powered by Codecov. Last updated by 5667c2d...11d146c

@stevvooe
Copy link
Contributor

I'd prefer we fix the todo... I leave in spammy messages for a reason. When the architecture is correct, this message goes away.

@dperny
Copy link
Collaborator Author

dperny commented Jun 24, 2016

I don't understand what the correct architecture should be. What is important about reporting progress? I don't understand the problem well enough to fix it.

@aluzzardi
Copy link
Member

Ping @stevvooe: could you address @dperny's question so we can move this forward?

@stevvooe
Copy link
Contributor

stevvooe commented Aug 5, 2016

I don't understand what the correct architecture should be. What is important about reporting progress? I don't understand the problem well enough to fix it.

We need to provide visibility into the pull process.

We can probably report this through container status. I'd like to see the changes proposed in docker/engine-api#89 (comment) and we can work from there.

We may also want to flow some of this information into the message field on TaskStatus.

@aluzzardi
Copy link
Member

I don't think we can use container status - it would put too much pressure into the store.

Actually, we're already sending too many updates - the agent eventually should probably compact those reports.

Writing to raft is about the most expensive thing we can do in the system.

@stevvooe
Copy link
Contributor

stevvooe commented Aug 6, 2016

Writing to raft is about the most expensive thing we can do in the system.

Agreed, but we need some visibility into pull.

the agent eventually should probably compact those reports.

The protocol already supports this. We just need to make the agent do it.

@dperny dperny closed this Aug 8, 2016
@dperny dperny deleted the 935-obnoxoius-logs-be-gone branch May 3, 2019 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants