trachten / cpisync

A library for synchronizing remote data with minimum communication.
GNU General Public License v3.0
26 stars 11 forks source link

createForkForTest does not use probSync parameter #70

Closed novakboskov closed 1 year ago

novakboskov commented 4 years ago
inline bool createForkForTest(GenSync& GenSyncClient, GenSync& GenSyncServer,bool oneWay, bool probSync,bool syncParamTest,
                               const unsigned int SIMILAR,const unsigned int CLIENT_MINUS_SERVER,
                               const unsigned int SERVER_MINUS_CLIENT, multiset<string> reconciled,
                               bool setofSets)

This apparently does not use probSync parameter. How does it check for the success of probabilistic syncs?

arorashu commented 4 years ago

I think the parameter should be removed, I don't think it is useful here.

For all syncs, the check is as described here:

At the end, GenSyncServer data is modified. The server is now expected to have the data - set A U B. For "one-way" syncs, the client should be unchanged, else the client is also modified to contain data - A U B If the resultant data set for GenSyncClient and GenSyncServer is as expected, the test passes, else the test fails.

So, just compare the resultant sets with the reconciled sets.

Previously, the check was

clientReconcileSuccess &= (multisetDiff(reconciled, resClient).size() < (SERVER_MINUS_CLIENT)
                                            && (clientReport.bytes > 0) && (resClient.size() > SIMILAR + CLIENT_MINUS_SERVER));

which didn't actually check the data after reconciliation. I remember in one of our discussions, Prof. Trachtenberg mentioned that we should just make the probability of probabilistic syncs failing very low. So I think the probSync parameter can be removed. I can do that.

If not, what do you think the correctness condition should be?

novakboskov commented 4 years ago

I remember @trachten mentioning that making failure probability for probabilistic syncs very low in testing is an option. In that case, just make a new pull request to address this change and make sure that you cleaned up the code. That is, make sure that probSync does not appear in the other places where it does not make sense.

arorashu commented 4 years ago

Fixed in PR#78