add dirty rpc function definition for sync data

windmemory commented 4 years ago

Currently, we don't have a way of force the wechaty-puppet-hostie server to clear it's cache, so we can sync the data from the IM server. I would like to add several rpc calls in the proto, so we can use it to clear server side cache.

windmemory commented 4 years ago

:( So hard to make Go / Build happy.

windmemory commented 4 years ago

@huan Could you please review this PR?

huan commented 4 years ago

Ok, I'll review it tonight.

The reason that I have not reviewed it for the past two days is that I think there are two ways that we could implement this new feature:

1. Top-down

From this PR I can see a design that we want to dirty a payload from top to down: the higher level puppet will call your dirtyPayload to trigger the payload update.

2. Bottom-up

However, I'm thinking about doing this from down to top. It would be better for the lower level puppet to be in charge of managing the payload dirty updates.

When a payload needs to be updated, the lower puppet will emit a dirty event for that payload, and this event will be propagated from lower-level puppets to the higher level puppets, and all payloads will be updated.

Conclusion

I did not think it very much yet so there might be some pros/cons between the top-down and bottom-up methods.

Please feel free to let me know what you are thinking about the bottom-up ways to archive this new feature, and I'll try to compare them later tonight.

windmemory commented 4 years ago

My PR includes both top-down and bottom-up design. The methods added are for the top-down sync, and the new events added are for the bottom-up sync.

If let me design the system, I would keep both top-down and bottom-up solutions, reasons are:

Bottom-up design is necessary, I think we are in consensus on this idea, so no more discussion on this
Top-down design is arguable. In my opinion, the top-down sync is unnecessary only in an ideal system, which means all data are managed by the underlying puppet and they are always in sync with remote. But in realistic, this is not possible for now, not all data can be perfectly synced. When data is not in sync and developer does not have a way to sync it, it will be really annoying. So we need to have a way to sync the data top-down
contact.sync() should sync the data from top-down and get the fresh data from the IM server, if we don't have a way to do top-down sync, then contact.sync() can not work as expected.
Current wechaty-puppet design supports both way of syncing data, and working pretty good so far, I would consider that this solves developer's problems.

So I would prefer to implement both top-down and bottom-up mechanism to keep the behavior or wechaty-puppet-hostie consistent with any other puppets.

huan commented 4 years ago

Thanks for your explanation, I'll investigate it more later tonight.

P.S. If we can describe in detail about the design of this PR at the same time that it was created, we will be able to speed up the communication loop from the beginning.

The puppet system is really important for the Wechaty ecosystem (which includes the GRPC design), that's why I'm always try my best to keep it as simple as possible.

Appreciate your time and I'll get back to our conversion soon.

huan commented 4 years ago

@windmemory Could you please help me to list all the cases which require the users to have to call the contact.sync() (and room.sync()) ?

I always keep thinking that whether we should treat the .sync() as a public API, because the right direction for us is to build an ideal system, which should not rely on a sync() method to sync data for the end-users.

contact.sync() should sync the data from top-down and get the fresh data from the IM server, if we don't have a way to do top-down sync, then contact.sync() can not work as expected.

windmemory commented 4 years ago

For contact, we might modify the alias on phone, and we want to get that data synced in wechaty, currently to achieve that, we need to sync the contact periodically.

For room, members might leave, or change their room alias, to get the latest data, we need to sync the room data.

The root cause for this design, is that not all changes of rooms or contacts are pushed to wechaty, and currently this is limited by the underlying api, so the sync() method is the workaround for the problem.

Do you have any better idea to solve this problem?

huan commented 4 years ago

@windmemory Thank you very much for those use cases, they are very persuasive.

So let's keep your two-direction design for this version.

After a good sleep, I believe we can reuse the event types (like EVENT_TYPE_DIRTY_CONTACT) to reduce our rpc method number.

Here's how:

Top-down

We call puppet.dirtyPayload(payloadType, payloadId) to proactive dirty a payload, from the higher wechaty layer.

Bottom-up

We emit a dirty event with payloadType and payloadId from the lowest puppet layer, and all the puppets must propagate this event to the higher layers and react to it to clean that dirty-ed payload.

Other Changes

We change all the xxxPayloadDirty() methods like contactPayloadDirty from public to protected, which means they are not a public API anymore.

All actions that want to dirty a payload need to call the new dirtyPayload() method after this update, to make the API clean.

Both the dirtyPayload() method and the dirty event listeners, they should be designed to call the xxxPayloadDirty() to execute the payload task.

Conclusion

After this refactoring, we will:

add one dirtyPayload() as the new public API for the puppet, update the payload from top-down
add one dirty event as a new event for puppet, update the payload from bottom-up
change the xxxPayloadDirty() method API from the public to protected.

Please feel free to let me know if you have any comments/suggestions/questions.

huan commented 4 years ago

@windmemory There is always a better way to make sure the CI will be happy: run tests locally! :)

windmemory commented 4 years ago

Local test shows different results from the CI build, not sure where went wrong, so I am fixing local test while pushing changes in parallel, I think this is the fastest way to fix the problem.

windmemory commented 4 years ago

Don't know how to fix the Go / Build... Need help

huan commented 4 years ago

The code looks great now! It's very clean and beautiful, thank you very much for the time discussing with me, this improvement is very valuable!

You can leave the Go part to me, I'll merge this PR later.

windmemory commented 4 years ago

🎉

huan commented 4 years ago

I bumped the version for you. :)

huan commented 4 years ago

Link to https://github.com/wechaty/wechaty-puppet-hostie/issues/43

wechaty / grpc