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

Audit file #14

Merged
merged 17 commits into from
Mar 27, 2018
Merged

Audit file #14

merged 17 commits into from
Mar 27, 2018

Conversation

dylankb
Copy link
Member

@dylankb dylankb commented Mar 27, 2018

Steps to Test:

  • go to the audit directory, and uncomment the line in audit/client/node.js for uploading a file
  • start server, and server2 nodes up in separate terminals.
  • after file is upload, uncomment line for retrieving a file. you'll need to put update the manifest file name as we aren't commit .env files to source control so your private key will generate a new file name for the manifest.
  • Your audit should pass!

Notes

  1. Naive auditing. Auditing currently checks that every node holding a redundant shard passes a data integrity check. This is a bit naive, but we need to decide if we want to do something else.

  2. Slow checks? The auditing check code that transforms the manifest and checks each shard duplicate seemed to slow running an audit down a bit.

  3. auditShardData has a lot of logic in it, so I was thinking of trying to bring some of it up higher, maybe like this, but interested in what you all think.

  auditShardsGroup(shards, shaIdx, shardAuditData) {
    let shardDupIdx = 0;
    let duplicatesAudited = 0;
    const shaId = Object.keys(shards)[shaIdx];

    while (shards[shaId].length > duplicatesAudited) {
      this.auditShard(shards, shardDupIdx, shaId, shaIdx, shardAuditData);
      duplicatesAudited += 1;
    }
  }
  1. Threw in a little JSdoc

batnode.js Outdated
}
}
})
}
Copy link
Member

Choose a reason for hiding this comment

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

I agree that the auditShardData contains a lot of logic and it will be good if we can move part of the logic elsewhere(like the example in your comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it took a little while but I agreed and I did that. See the latest commit.

@dylankb dylankb assigned WilfredTA and unassigned dylankb Mar 27, 2018
@dylankb
Copy link
Member Author

dylankb commented Mar 27, 2018

One thing that it seems like I should also do now that I think of it is make use of the more generalized getHostNode. I thought you could just pass in a callback that relied on a batNode param already resolved, but this

this.getHostNode(shaId, (batNode) => {
  this.auditShardData(batNode, shards, shaIdx, shardDupIdx, shardAuditData)
});

and this both didn't work. Am I not using your getHostNode method right, @WilfredTA ?

const auditShardDataWith = (batNode) => {
  this.auditShardData(batNode, shards, shaIdx, shardDupIdx, shardAuditData)
};
this.getHostNode(shaId, auditShardDataWith);

Most recent commit has this code, and it breaks audit. Use previous commit if you want to test it out when it works.

@floalex
Copy link
Member

floalex commented Mar 27, 2018

Looks good for the changes.
The thumbs up is for OK to merge.

@WilfredTA
Copy link
Member

@dylankb It looks like you're still using auditShard instead of auditShardData with getHostNode.

I know that there was one method you were using in which the callback passed to getHostNode was pre-defined was working. I think using getHostNode in conjunction with a callback that calls auditShardData and then deleting auditShard is a better way to go, because auditShard introduces a lot of code duplication.

@dylankb
Copy link
Member Author

dylankb commented Mar 27, 2018

Yeah, I had just reverted back to the working code so maybe the branch UI hadn't refreshed because I no longer am trying to use getHostNode.

Yes, I agree that what you're saying is the goal. We did try and do more less that in the live on our call, but for some reason it caused problems so I'm leaving a note in this issue to follow up on it later.

@dylankb
Copy link
Member Author

dylankb commented Mar 27, 2018

@WilfredTA Let me know if you want to give this a 👍 or if you want me to make any additional changes.

Copy link
Member

@floalex floalex left a comment

Choose a reason for hiding this comment

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

OK to merge

@dylankb dylankb merged commit 2c72ae2 into master Mar 27, 2018
@dylankb dylankb deleted the audit-file branch March 30, 2018 19:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants