Persistent message map #1996

yousefmansy1 · 2023-03-01T09:45:33Z

Persistent message map
Resolves #541

yousefmansy1 · 2023-03-01T09:53:37Z

@42wim With all the open PRs I have the branches are starting to get messy, can we start to get some of the PRs merged? (Whenever you're available)
Some things kind of depend on others. As of now my test branch has a pretty clean merge of all of my open PRs and is what I'm running personally.

codeclimate

The PR diff size of 22952 lines exceeds the maximum allowed for the inline comments feature.

yousefmansy1 · 2023-03-01T10:07:27Z

gateway/gateway.go

@@ -59,12 +63,41 @@ func New(rootLogger *logrus.Logger, cfg *config.Gateway, r *Router) *Gateway {
 	if err := gw.AddConfig(cfg); err != nil {
 		logger.Errorf("Failed to add configuration to gateway: %#v", err)
 	}
+
+	persistentMessageStorePath, usePersistent := gw.Config.GetString("PersistentMessageStorePath")


Am I getting the config correctly here?
Unless I set it at the very top outside of any nesting it doesn't seem to work.

@yousefmansy1 I'm merging them, thanks for your work on those PRs and sorry for the delays.

Thanks @42wim!
No worries at all, I'm sure you had a busy schedule.

As for how we're using getting config values, I have to put this value at a top level or else value comes back as nil when I call gw.Config.GetString("PersistentMessageStorePath")
eg:

PersistentMessageStorePath="/etc/matterbridge/badger" [discord] [discord.bot] ... [telegram] [telegram.bot] ... [whatsapp] [whatsapp.googlevoice1] ... [general] ...

Given this is a gateway level config value it should be under [general] or any of the other gateway configs.

You should use gw.BridgeValues().General.PersistentMessageStorePath and add the setting to General in bridge/config/config.go

Resolved in 60219a3

Let me know if you hate the formatting applied in config, and I can undo.

42wim · 2023-03-09T21:12:01Z

@42wim With all the open PRs I have the branches are starting to get messy, can we start to get some of the PRs merged? (Whenever you're available) Some things kind of depend on others. As of now my test branch has a pretty clean merge of all of my open PRs and is what I'm running personally.

@yousefmansy1 I'm merging them, thanks for your work on those PRs and sorry for the delays.

42wim

Haven't tested it yet, just quick review with some remarks

42wim · 2023-03-10T22:33:16Z

gateway/gateway.go


 	logger *logrus.Entry
 }

 type BrMsgID struct {
-	br        *bridge.Bridge
-	ID        string
+	Protocol  string


why removing the br ?

We are directly storing these values in our persistent store, storing a reference to br, would just be garbage data when we read it back.
Based on the current usage of br for this struct we are only using it to get the protocol and destination gateway name.

matterbridge/gateway/gateway.go

Lines 267 to 269 in 8587fa8

if dest.Protocol == id.br.Protocol && dest.Name == id.br.Name && channel.ID == id.ChannelID {

return strings.Replace(id.ID, dest.Protocol+" ", "", 1)

}

https://github.com/yousefmansy1/matterbridge/blob/c0f5d0c5f7a5c7c9edd871bea1cc07ebadbcdb1a/gateway/gateway.go#L313-L315

For this persistent feature we are removing this reference and storing the direct values.
Unfortunately, that does mean if you change the name of the gateway things will break.

Sure, I understand not storing the br reference it self, but you can still reference br.Protocol and br.Name instead of creating those 2 variables? Or am I missing something about this?

For the in memory cache yes.
But for the persistent message map, if we restart the application all the memory references we have stored inside our value store will point whatever is the old br reference, which would be different for each run of the application.

I could be misunderstanding something about how references work in golang, correct me if I'm wrong.

if you actually store br it's a reference, but br.Protocol or br.Name is a string

Oh ok I see. Yes, that would make more sense and would make its functionality more clear.
Will update.

Actually maybe I'm misunderstanding

type BrMsgID struct { Protocol bridge.Bridge.Protocol DestName bridge.Bridge.Name ChannelID string ID string }

causes build errors:

yousef@DESKTOP-YOUSEF $ go build -tags whatsappmulti -gcflags=all="-N -l" # github.com/42wim/matterbridge/gateway gateway/gateway.go:40:25: syntax error: unexpected . in struct type; possibly missing semicolon or newline or }

BrMsgID stays the same with br *bridge.Bridge, where you need the protocol just use br.Protocol
I'm doing this on mobile so I could also be misunderstanding your issue

I might also be misunderstanding what you mean, but this change is most important in this code snippet

https://github.com/yousefmansy1/matterbridge/blob/60219a39d1624df64de9421177b326bde269319f/gateway/persistent.go#L59-L74

When we get the []BrMsgID from the message store, anything that we read from those items cant come from any reference.

42wim · 2023-03-10T22:40:48Z

gateway/gateway.go

@@ -59,12 +63,41 @@ func New(rootLogger *logrus.Logger, cfg *config.Gateway, r *Router) *Gateway {
 	if err := gw.AddConfig(cfg); err != nil {
 		logger.Errorf("Failed to add configuration to gateway: %#v", err)
 	}
+
+	persistentMessageStorePath, usePersistent := gw.Config.GetString("PersistentMessageStorePath")


You should use gw.BridgeValues().General.PersistentMessageStorePath and add the setting to General in bridge/config/config.go

gateway/gateway.go

gateway/persistent.go

42wim · 2023-03-10T22:47:50Z

gateway/persistent.go

+		gw.logger.Error(err)
+	}
+
+	if found {


return early

Suggested change

if found {

if !found {

return ""

}

I feel like that early return is less or at best equivalently inuitive.

The new code would probably loook like:

if ! found { return "" } for _, id := range *destMessageIds { // check protocol, bridge name and channelname // for people that reuse the same bridge multiple times. see #342 if dest.Protocol == id.Protocol && dest.Name == id.DestName && channel.ID == id.ChannelID { return id.ID } } return ""

not really an improvement IMHO.

42wim · 2023-03-10T22:52:07Z

gateway/gateway.go

@@ -29,14 +30,17 @@ type Gateway struct {
 	Message        chan config.Message
 	Name           string
 	Messages       *lru.Cache
+	MessageStore   gokv.Store
+	CanonicalStore gokv.Store


Is there a reason it can't be in 1 file?

Yes, in order to add this feature without significant refactor we are still following the whole concept of a "canonical" message.

We need two separate mappings one for message->canonical (CanonicalStore) and another for canonical->message[] (MessageStore)

This is all highly related to this another PR:
#1991 (comment)
#1991 (comment)

On the file system the directories look like this:

Each bridge gets its own subdirectory

Sure, but it's a k/v store, why can't you just put it in 1 file and use a "canonical" and "messages" prefix for the keys?

I mean we can I suppose, I don't really see how that's beneficial.

I feel the code is more readable to have distinct mappings for their functionality?
Plus, removes the risk of breaking old message stores by needing to change a hypothetical prefix.

What do you think?

seems like I didn't see that you said a subdirectory per bridge.

In my opinion it's much nicer to have everything in 1 file instead of a lot of files. So I'm even proposing to just have one database containing everything.

Particularly for the subdirectories per bridge, I prefer that solution as its more flexible.

Flexible in the manner, that each folder is as if its its own table.
If a user needs to rename a bridge and don't want to lose their historical data they can just rename the directory.

Additionally Badger db does not support buckets/tables for each keystore so sub directories is really the only way to do it.
for example what I did in another PR with bbolt:
https://github.com/yousefmansy1/matterbridge/blob/5353b32c1a21d2655f8e12e76f81628373acd6f5/gateway/persistent.go#L21-L34

The way I'd like to treat it closer to a set of tables in a DB rather than one big KV store where we dump things in and "filter" with prefixes.
For the non technical user they can just treat the root directory as the whole db and ignore it.

By the way unless we're always closing and opening the DB/KV handler with each Read/Write we do need individual stores for each one as each KV store actually holds a lock over the store. No other stores will be able to open it until it closes it's connection.
Its probably a little stupid but we'll never run into this blocking if each gateway has its own store it keeps for itself.

(also reformatted and sorted the config struct)

codeclimate

The PR diff size of 23162 lines exceeds the maximum allowed for the inline comments feature.

codeclimate · 2023-03-11T23:43:28Z

Code Climate has analyzed commit 60219a3 and detected 0 issues on this pull request.

View more on Code Climate.

oizyumov · 2023-03-21T12:22:39Z

@42wim could you return back here, please (:
The implementation of the persistent message map seems more than a significant upgrade to matterbridge!

sas1024 · 2023-06-08T09:36:19Z

@42wim any news with PR review?

sas1024 · 2023-07-21T08:08:54Z

@42wim ?

42wim and others added 2 commits March 1, 2023 01:35

Bump version

0527f01

Add persistent message map

c0f5d0c

Resolves 42wim#541

yousefmansy1 mentioned this pull request Mar 1, 2023

Allow message deletes from any platform #1991

Open

yousefmansy1 force-pushed the persistent-message-map branch from c0f5d0c to 890967e Compare March 1, 2023 09:52

codeclimate bot reviewed Mar 1, 2023

View reviewed changes

yousefmansy1 force-pushed the persistent-message-map branch from 890967e to c0f5d0c Compare March 1, 2023 09:56

yousefmansy1 commented Mar 1, 2023

View reviewed changes

42wim reviewed Mar 10, 2023

View reviewed changes

yousefmansy1 added 3 commits March 11, 2023 14:33

Add descriptive errors to persistent store

eaa8907

Pass peristent store errors up to the router level

2845740

get PersistentMessageStorePath config properly

60219a3

(also reformatted and sorted the config struct)

codeclimate bot reviewed Mar 11, 2023

View reviewed changes

yousefmansy1 requested a review from 42wim March 18, 2023 02:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persistent message map #1996

Persistent message map #1996

yousefmansy1 commented Mar 1, 2023

yousefmansy1 commented Mar 1, 2023

codeclimate bot left a comment

yousefmansy1 Mar 1, 2023

yousefmansy1 Mar 10, 2023 •

edited

Loading

42wim Mar 10, 2023

yousefmansy1 Mar 11, 2023

42wim commented Mar 9, 2023

42wim left a comment

42wim Mar 10, 2023

yousefmansy1 Mar 11, 2023

42wim Mar 11, 2023

yousefmansy1 Mar 11, 2023

42wim Mar 11, 2023

yousefmansy1 Mar 11, 2023

yousefmansy1 Mar 11, 2023

42wim Mar 11, 2023

yousefmansy1 Mar 12, 2023

42wim Mar 10, 2023

42wim Mar 10, 2023

yousefmansy1 Mar 11, 2023

42wim Mar 10, 2023

yousefmansy1 Mar 11, 2023 •

edited

Loading

42wim Mar 11, 2023 •

edited

Loading

yousefmansy1 Mar 11, 2023

42wim Mar 11, 2023

yousefmansy1 Mar 12, 2023 •

edited

Loading

yousefmansy1 Mar 23, 2023

codeclimate bot left a comment

codeclimate bot commented Mar 11, 2023

oizyumov commented Mar 21, 2023

sas1024 commented Jun 8, 2023

sas1024 commented Jul 21, 2023

	if dest.Protocol == id.br.Protocol && dest.Name == id.br.Name && channel.ID == id.ChannelID {
	return strings.Replace(id.ID, dest.Protocol+" ", "", 1)
	}

-	if found {
+	if !found {
+	   return ""
+	}

Persistent message map #1996

Are you sure you want to change the base?

Persistent message map #1996

Conversation

yousefmansy1 commented Mar 1, 2023

yousefmansy1 commented Mar 1, 2023

codeclimate bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yousefmansy1 Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

42wim commented Mar 9, 2023

42wim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yousefmansy1 Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

42wim Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yousefmansy1 Mar 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot left a comment

Choose a reason for hiding this comment

codeclimate bot commented Mar 11, 2023

oizyumov commented Mar 21, 2023

sas1024 commented Jun 8, 2023

sas1024 commented Jul 21, 2023

yousefmansy1 Mar 10, 2023 •

edited

Loading

yousefmansy1 Mar 11, 2023 •

edited

Loading

42wim Mar 11, 2023 •

edited

Loading

yousefmansy1 Mar 12, 2023 •

edited

Loading