Skip to content

design: add client.md#1573

Closed
AkihiroSuda wants to merge 1 commit intocontainerd:masterfrom
AkihiroSuda:design-client
Closed

design: add client.md#1573
AkihiroSuda wants to merge 1 commit intocontainerd:masterfrom
AkihiroSuda:design-client

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

Signed-off-by: Akihiro Suda [email protected]

@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 29, 2017

Codecov Report

Merging #1573 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1573   +/-   ##
=======================================
  Coverage   48.58%   48.58%           
=======================================
  Files          28       28           
  Lines        4250     4250           
=======================================
  Hits         2065     2065           
  Misses       1750     1750           
  Partials      435      435

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17093c2...0689e63. Read the comment docs.

Comment thread design/client.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think its weird to add documents like this for only one item. Either do them all or none at all. It just creates weird documents that are narrow and confuse the readers.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is there other item?

Using listing instead of table might be better?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll add GC labels after #1563

Comment thread design/client.md Outdated
Copy link
Copy Markdown
Member

@stevvooe stevvooe Oct 3, 2017

Choose a reason for hiding this comment

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

Want to add a TODO here with more information about how containerd is a client-heavy design?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we assuming the reader should know the pros and cons of gRPC and how it lends itself to implementing 3rd-party containerd clients? It may be helpful to provide an example.

Copy link
Copy Markdown
Member

@stevvooe stevvooe Oct 3, 2017

Choose a reason for hiding this comment

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

We've already made the choice. ;)

@estesp has a talk somewhere where he discusses the decision. There are also some old discussions about how avoiding http specification time speeds up implementation.

A link to grpc.io would be good.

Comment thread design/client.md Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are push&pull interactions the only thing a 3rd-party client would need to reimplement?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, there is a huge list. We should probably document that list here.

@AkihiroSuda
Copy link
Copy Markdown
Member Author

Updated PR, including GC labels
cc @dmcgowan

Comment thread design/client.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These aren't restricted to these types, are they?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@dmcgowan PTAL (why do we need timestamp here?)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@AkihiroSuda The timestamp tells you when the label expires, letting you reserve objects before they are referenced elsewhere.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right now I am using the creation time. While garbage collection is done by the serve, a "prune" operation will be client side and these timestamps can be used to hint any client doing pruning as to which objects may be old. It is a client decision but by adding a timestamp here it gives clients more information it may need. This value is not read by the server, that should be noted, it is only for client use.

Comment thread design/client.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also allow gc.ref.content.${something} to allow adding multiple links. It would be inaccurate to say "Digest string of children blobs." since each label may only refer to a single digest. To refer to multiple children multiple label keys must be used using a client define string after gc.ref.content..

@AkihiroSuda
Copy link
Copy Markdown
Member Author

updated PR
@dmcgowan

Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda
Copy link
Copy Markdown
Member Author

@dmcgowan PTAL?

@stevvooe
Copy link
Copy Markdown
Member

LGTM

I still think this doc needs a much large expansion, but that might be on me.

@crosbymichael
Copy link
Copy Markdown
Member

Same for me. Its a weird document.

@stevvooe
Copy link
Copy Markdown
Member

I'd feel more comfortable with this as part of the package doc, maybe.

@AkihiroSuda AkihiroSuda added this to the 1.0.0 post release milestone Dec 5, 2017
@crosbymichael
Copy link
Copy Markdown
Member

I'm just going to close this PR. It's not adding any real value to the reader and adds more confusion in turn. It's just random information with no real consistency for a design document and is not up to par with the other design documents that are in depth.

@AkihiroSuda
Copy link
Copy Markdown
Member Author

Could you open an alternate PR?

@stevvooe
Copy link
Copy Markdown
Member

@AkihiroSuda I'd suggest adding this to the godoc, where it makes sense to document details of working with the Go client.

mauriciovasquezbernal pushed a commit to kinvolk/containerd that referenced this pull request Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants