Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

api, chunk, storage: anonymous uploads #1828

Merged
merged 28 commits into from
Oct 8, 2019
Merged

api, chunk, storage: anonymous uploads #1828

merged 28 commits into from
Oct 8, 2019

Conversation

acud
Copy link
Member

@acud acud commented Sep 27, 2019

No description provided.

@acud acud added this to the 0.5.0 milestone Sep 27, 2019
@acud acud self-assigned this Sep 27, 2019
@zelig
Copy link
Member

zelig commented Sep 27, 2019

Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

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

LGTM already.

  • not sure enough test coverage
  • does anonymous header matter for pinned upload too?
  • sync mode should be eliminated now its not checked right?
  • full cli support
  • documentation

api/http/server_test.go Show resolved Hide resolved
@@ -131,8 +132,8 @@ func InitUploadTag(h http.Handler, tags *chunk.Tags) http.Handler {
}

log.Trace("creating tag", "tagName", tagName, "estimatedTotal", estimatedTotal)

t, err := tags.Create(tagName, estimatedTotal)
anon := anonTag == "true"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe to make it case insensitive?

chunk/chunk.go Show resolved Hide resolved
storage/localstore/mode_set.go Outdated Show resolved Hide resolved
case chunk.ModeSetSyncPull:
if tag.Anonymous {
// this will not get called twice because we remove the item in L217
tag.Inc(chunk.StateSent)
Copy link
Member

Choose a reason for hiding this comment

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

I would improve this function to increment tag counters only if the batch is successfully written, probably returning bool variables that would be handled in set method.

This can be left for later when tags are persisted. Without persistence, I do not think that it matters.

pushsync/pusher.go Outdated Show resolved Hide resolved
@acud
Copy link
Member Author

acud commented Oct 1, 2019

  • not sure enough test coverage

Working on it

  • does anonymous header matter for pinned upload too?

Well:

  • If anonymous && pin then enter pull index and stay, but since we went with this approach of hacking around the current indexes we must add more plumbing to prevent the items from entering the push index to not get collected by pushsync (then you lose the progress bar functionality for pull sync)
  • If !anonymous && pin then enter both indexes as today, push sync pushes and removes from push index, pull sync pinned content stays
  • full cli support
  • documentation

Once the code works - gladly

@zelig
Copy link
Member

zelig commented Oct 1, 2019

Well:

* If anonymous && pin then enter pull index and stay, but since we went with this approach of hacking around the current indexes we must add more plumbing to prevent the items from entering the push index to not get collected by pushsync (then you lose the progress bar functionality for pull sync)

* If !anonymous && pin then enter both indexes as today, push sync pushes and removes from push index, pull sync pinned content stays

This is beyond me @acud . Would you walk me through this?

storage/localstore/mode_set_test.go Outdated Show resolved Hide resolved
storage/localstore/mode_set_test.go Show resolved Hide resolved
storage/localstore/mode_set_test.go Outdated Show resolved Hide resolved
@skylenet skylenet modified the milestones: 0.5.1, 0.5.2 Oct 3, 2019
@zelig
Copy link
Member

zelig commented Oct 7, 2019

@acud acud force-pushed the anonymous-tags branch 3 times, most recently from d11164f to e04f33e Compare October 7, 2019 07:03
storage/localstore/localstore_test.go Outdated Show resolved Hide resolved
storage/localstore/localstore_test.go Outdated Show resolved Hide resolved
storage/localstore/mode_set.go Outdated Show resolved Hide resolved
storage/localstore/localstore_test.go Outdated Show resolved Hide resolved
api/api_test.go Show resolved Hide resolved
storage/localstore/localstore_test.go Outdated Show resolved Hide resolved
storage/localstore/localstore_test.go Show resolved Hide resolved
storage/localstore/localstore_test.go Show resolved Hide resolved
storage/localstore/mode_set_test.go Outdated Show resolved Hide resolved
storage/localstore/mode_set_test.go Show resolved Hide resolved
@janos
Copy link
Member

janos commented Oct 8, 2019

The linter is still complaining https://travis-ci.org/ethersphere/swarm/jobs/594965019#L490.

Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

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

Thanks @acud. LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants