emichael / dslabs

Distributed Systems Labs and Framework
https://ellismichael.com/dslabs/
1.26k stars 341 forks source link

Add lombok annotations for ShardStoreNode #45

Closed sgdxbc closed 1 year ago

sgdxbc commented 1 year ago

Just helped debug a student solution who put some states into ShatdStoreNode, and this diff solved it. The situation was quite frustrating: all tests except BFS can pass, BFS failed by running out of time when searching goal state (so there's no valuable trace). The goal state seems to be missed because of non-deterministic caused by incorrect equal and hash methods, but --checks reports nothing, and I'm not sure why. Maybe it need to be fixed as well.

README currently says nothing about we cannot put states into ShardStoreNode. While stating that is an alternative approach, I feel it would be better if we can offer more flexibility to students.

The annotations do not break my own solution, and I think it probably will not break any other reasonable solution.

emichael commented 1 year ago

I don't mind having @EqualsAndHashCode here. I think that's probably the right thing to do. But I don't really like including @ToString since it just further pollutes the toString for people who aren't storing any mutable state in ShardStoreNode.

Btw, --checks wouldn't have caught this problem. It can only find situations where states that should be equal aren't. It can't do anything about states which shouldn't be equal; how could we know?

sgdxbc commented 1 year ago

I see the point about --checks. Previously I thought that since every ShardStoreNode is not equal to each other, its subclasses who callSuper in their equal methods will never have positive result. But if the case is that the same super class get reused all the time then this should be the case.

About @ToString I just checked the visualizer. There are only two more items shown for server: address and subNodes, and the latter one is by default folded, so in my opinion it's not too cluttered. This may not be the case when print the node directly to console, but I suspect anyone would really do so except --checks (whose error message is already very non-readable anyway). After all, we can disable including subnodes into @ToString result in framework. Should I do that?

emichael commented 1 year ago

There are plenty of times when nodes' toString() are printed to the console. I don't want @ToString on ShardStoreNode. If anyone adds state to ShardStoreNode (which they probably shouldn't be doing), they can add toString() information themselves.

sgdxbc commented 1 year ago

Ok, I can agree with you. Updated.

danielzting commented 1 year ago

Put configs in ShardStoreNode since both ShardStoreClient and ShardStoreServer need it, spent 10 hours debugging this single problem...though I took this class too early to benefit from this PR, many thanks on behalf of future students 🥲

emichael commented 1 year ago

FYI, that would have been caught if you ran with --checks

danielzting commented 1 year ago

I thought you mentioned --checks can only ensure states that should be equal are? When I tried it with my professor in office hours we only got a report that handlePaxosRequest was not idempotent

emichael commented 1 year ago

Oh, right. Totally forgot about the context. There was an equals method already.