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

go-ipfs-files 2.0 updates #613

Merged
merged 8 commits into from Dec 17, 2018
Merged

go-ipfs-files 2.0 updates #613

merged 8 commits into from Dec 17, 2018

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Dec 1, 2018

These are updates for changes in ipfs/go-ipfs-files#2

@GitCop
Copy link

GitCop commented Dec 1, 2018

There were the following issues with your Pull Request

  • Commit: 2ffb411
  • Invalid signoff. Commit message must end with
    License: MIT
    Signed-off-by:

Code contribution guidelines for IPFS Cluster are available at https://cluster.ipfs.io/developer/contribute/#code-contribution-guidelines


This message was auto-generated by https://gitcop.com

@coveralls
Copy link

coveralls commented Dec 1, 2018

Coverage Status

Coverage decreased (-0.02%) to 65.09% when pulling 11cc048 on gx/files2 into e039c71 on master.

Copy link
Collaborator

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

Hey, thanks for this PR, it will save us time.

TestAdder_ContextCancelled fails, but probably is just racy

No, I think it's due to the change. A test that never failed before, failing on both travis runs, which is relevant to the code modified by this PR... I might be wrong but I think you should look closer.

Additionally Jenkins fails:

sharding.go:141: remove shardTesting\testTree\B\big_file: The process cannot access the file because it is being used by another process.

This likely means that the readers are not being closed like before between tests (or in the same test).

Allow me to be a bit critic of the 2.0.0 change: for us, it does not simplify code, it seems to introduce some breakage and it costs us some time. Not against it, I understand it will make your life easier on some other place, but still. I really appreciate that you took the time to submit the change-set here though. Can you have another look and see why that context error is not coming back anymore? I can help you too in a few days, but I'd suggest not merging until we have figured out if the issues are in cluster's land or in files' land. It wouldn't be the first time that Cluster tests catch some issue in dependencies, it might be just cluster.

adder/adder.go Outdated Show resolved Hide resolved
adder/ipfsadd/add.go Outdated Show resolved Hide resolved
adder/ipfsadd/add.go Outdated Show resolved Hide resolved
adder/ipfsadd/add.go Outdated Show resolved Hide resolved
test/sharding.go Show resolved Hide resolved
defer closer.Close()
defer st.Close()

slf := files.NewSliceFile([]files.DirEntry{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test should not need to adder to be interating over a SliceFile. Can you explain the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A change in adder.FromFiles changed this behaviour slightly:

Before the change in https://github.com/ipfs/ipfs-cluster/pull/613/files?utf8=%E2%9C%93&diff=split#diff-a2c1e35d2137b0f7d1899f06c11d87b6L136 we would:

  • Enter into the loop
  • Check context (successfully)
  • Call NextFile and process it
  • Loop again
  • Check context (timeout in test), return

After the change:

  • Enter into the loop, calling Next
  • Check context (successfully)
  • Process the file
  • Exit loop as Next returns false (note that we don't check context as we don't re-enter the loop)
  • Exit FromFiles successfully as nothing else used the context

Strictly speaking old behaviour is more correct, but since we added the file fully in both cases, I'd argue that it makes sense to just return the result instead of timing out since we are already on the return path either way.

I needed to make this test take more than one file in order to force the loop to go around at least twice

@magik6k magik6k force-pushed the gx/files2 branch 3 times, most recently from c53e5d3 to 91b9426 Compare December 3, 2018 17:06
@magik6k
Copy link
Contributor Author

magik6k commented Dec 3, 2018

Addressed the review and made CI happier. Do you mind giving this / ipfs/go-ipfs-files#2 another pass?

@ghost ghost assigned hsanjuan Dec 6, 2018
Copy link
Collaborator

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

Let's leave this here until there is a final dep hash for files.

@hsanjuan
Copy link
Collaborator

hsanjuan commented Dec 6, 2018

And really, thanks a lot @magik6k !

magik6k and others added 6 commits December 17, 2018 14:16
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
@hsanjuan hsanjuan merged commit 9f6a173 into master Dec 17, 2018
@ghost ghost removed the status/in-progress In progress label Dec 17, 2018
@hsanjuan hsanjuan deleted the gx/files2 branch December 17, 2018 14:47
@hsanjuan hsanjuan mentioned this pull request Dec 17, 2018
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.

None yet

4 participants