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

[PHT] Key consistency #78

Merged
merged 8 commits into from Jul 12, 2016

Conversation

Projects
None yet
4 participants
@sim590
Contributor

sim590 commented Jun 15, 2016

This PR enhances the Pht API:

  • A Key is now std::map<std::string, Blob> rather than std::map<std::string, Prefix>.

  • The Pht now depends on a key specification

    using KeySpec = std::map<std::string, size_t>
    

    This key specification sets the order, the number and the length of the fields in the key.

Other minor changes:

  • dhtnode will handle the key spec by setting the keyspec for the pht with the first key entered by the user;
  • the benchmark has been updated accordingly.

@sim590 sim590 assigned aberaud, atraczyk and kaldoran and unassigned atraczyk Jun 15, 2016

@sim590 sim590 added this to the Pht indexation milestone Jun 15, 2016

@sim590 sim590 added the enhancement label Jun 15, 2016

@kaldoran

This comment has been minimized.

Show comment
Hide comment
@kaldoran

kaldoran Jun 15, 2016

Collaborator

Strange that travis can't compile it, got no problem on local copy of your rep.

Edit : Code seams all ok for me +1 for this patch 👊

Collaborator

kaldoran commented Jun 15, 2016

Strange that travis can't compile it, got no problem on local copy of your rep.

Edit : Code seams all ok for me +1 for this patch 👊

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 15, 2016

Contributor

@kaldoran: Travis doesn't use the same compiler version as yours then. And now, travis is happy.

@kaldoran, @aberaud : Since this is an API change, I would like some feedback. What do you think about the key spec implementation? I also have moved Pht::linearize into public:. I did this to provide a way to generate a prefix from a Key for displaying in the dhtnode. Do you think Pht::linearize should stay private:? Actually, more I think about it and more I think that Prefix should not be exposed in any way and only keys should be exposed in the API since a Prefix is literally the Pht internal representation of a Key. The user shouldn't be aware of the existence of the Prefix class. What do you think?

  • Hide Prefix from the API and only show the Key?
Contributor

sim590 commented Jun 15, 2016

@kaldoran: Travis doesn't use the same compiler version as yours then. And now, travis is happy.

@kaldoran, @aberaud : Since this is an API change, I would like some feedback. What do you think about the key spec implementation? I also have moved Pht::linearize into public:. I did this to provide a way to generate a prefix from a Key for displaying in the dhtnode. Do you think Pht::linearize should stay private:? Actually, more I think about it and more I think that Prefix should not be exposed in any way and only keys should be exposed in the API since a Prefix is literally the Pht internal representation of a Key. The user shouldn't be aware of the existence of the Prefix class. What do you think?

  • Hide Prefix from the API and only show the Key?

sim590 added some commits Jun 15, 2016

pht: add key spec, handling maximum prefix length
This commit adds the new KeySpec in the Pht constructor signature so that all
keys used inside a same PHT structure have the same maximal length.
python: update cython wrapper and python benchmark
API changed when the keys pec was added. This commit updates python code
according to the new API.
@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 17, 2016

Contributor

I've been thinking about hiding Prefix from the API and I think this is not a good thing since it would prevent the user to cumulate information about the state of the PHT. For e.g., the Prefix makes it possible to draw the trie formed on network.

  • However, there should be a more user friendly signature that doesn't show the Prefix:

    using LookupCallbackSimple = std::function<void(std::vector<std::shared_ptr<Value>>& values)>;
    using LookupCallback = std::function<void(std::vector<std::shared_ptr<Value>>& values, Prefix p)>;
    
    void lookup(Key k, LookupCallback cb = {}, DoneCallbackSimple doneCb = {}, bool exact_match = true);
    void lookup(Key k, LookupCallbackSimple cb = {}, DoneCallbackSimple doneCb = {}, bool exact_match = true) {
        lookup(k, [=](std::vector<std::shared_ptr<Value>>& values, Prefix p) { cb(values); }, doneCb, exact_match);
    }
Contributor

sim590 commented Jun 17, 2016

I've been thinking about hiding Prefix from the API and I think this is not a good thing since it would prevent the user to cumulate information about the state of the PHT. For e.g., the Prefix makes it possible to draw the trie formed on network.

  • However, there should be a more user friendly signature that doesn't show the Prefix:

    using LookupCallbackSimple = std::function<void(std::vector<std::shared_ptr<Value>>& values)>;
    using LookupCallback = std::function<void(std::vector<std::shared_ptr<Value>>& values, Prefix p)>;
    
    void lookup(Key k, LookupCallback cb = {}, DoneCallbackSimple doneCb = {}, bool exact_match = true);
    void lookup(Key k, LookupCallbackSimple cb = {}, DoneCallbackSimple doneCb = {}, bool exact_match = true) {
        lookup(k, [=](std::vector<std::shared_ptr<Value>>& values, Prefix p) { cb(values); }, doneCb, exact_match);
    }

sim590 added some commits Jun 17, 2016

pht: provide simpler lookup callback
From a user point of view, the structure Prefix doesn't make sense really. It's
better to provide a simpler callback function signature for the user then.
Show outdated Hide outdated tools/dhtnode.cpp
@kaldoran

This comment has been minimized.

Show comment
Hide comment
@kaldoran

kaldoran Jun 28, 2016

Collaborator

Since my PR #85 was closed ( and totaly need to be close )
There is the change i've made, you can cherry-pick here
https://github.com/kaldoran/opendht/commits/pht-api-fix
Commit : ffee145

Collaborator

kaldoran commented Jun 28, 2016

Since my PR #85 was closed ( and totaly need to be close )
There is the change i've made, you can cherry-pick here
https://github.com/kaldoran/opendht/commits/pht-api-fix
Commit : ffee145

@kaldoran

This comment has been minimized.

Show comment
Hide comment
@kaldoran

kaldoran Jun 28, 2016

Collaborator

Since you correct a mispell into the function isActiveBit, and since the bug is the same as was into PR #85 , could you also cherry-pick the commit that correct it here :
https://github.com/kaldoran/opendht/tree/pht-activebit-fix
Commit : fdbcb7a

Collaborator

kaldoran commented Jun 28, 2016

Since you correct a mispell into the function isActiveBit, and since the bug is the same as was into PR #85 , could you also cherry-pick the commit that correct it here :
https://github.com/kaldoran/opendht/tree/pht-activebit-fix
Commit : fdbcb7a

@kaldoran

This comment has been minimized.

Show comment
Hide comment
@kaldoran

kaldoran Jun 29, 2016

Collaborator

By the look of it, it seams that i gave you the wrong commit number ...
Sorry for that here is the correct one ( that compile )
https://github.com/kaldoran/opendht/commits/pht-api-fix
53dd544

Not sure if you can remove the previous commit of not.

Collaborator

kaldoran commented Jun 29, 2016

By the look of it, it seams that i gave you the wrong commit number ...
Sorry for that here is the correct one ( that compile )
https://github.com/kaldoran/opendht/commits/pht-api-fix
53dd544

Not sure if you can remove the previous commit of not.

@kaldoran

This comment has been minimized.

Show comment
Hide comment
@kaldoran

kaldoran Jun 29, 2016

Collaborator

I've made all the needed for the compilation error and correct the algorithm of linearize :)
The commit to cherry-pick is updated

Should have wait 5 second before push the fix of cmpilation error :)

Collaborator

kaldoran commented Jun 29, 2016

I've made all the needed for the compilation error and correct the algorithm of linearize :)
The commit to cherry-pick is updated

Should have wait 5 second before push the fix of cmpilation error :)

@kaldoran

This comment has been minimized.

Show comment
Hide comment
@kaldoran

kaldoran Jun 30, 2016

Collaborator

Wow, what append on this compilation but clang succeed and gcc does not, seams that this is a bug due to travis

Collaborator

kaldoran commented Jun 30, 2016

Wow, what append on this compilation but clang succeed and gcc does not, seams that this is a bug due to travis

@kaldoran kaldoran merged commit 4c649a8 into savoirfairelinux:index Jul 12, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@kaldoran

This comment has been minimized.

Show comment
Hide comment
@kaldoran

kaldoran Jul 12, 2016

Collaborator

I merge it since that compile on clang ( and should do the same of gcc without the pull error on travis ).

Collaborator

kaldoran commented Jul 12, 2016

I merge it since that compile on clang ( and should do the same of gcc without the pull error on travis ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment