Skip to content
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

Migrate from pg2 to pg (OTP 23) #47

Closed
alisinabh opened this issue Jun 30, 2020 · 12 comments
Closed

Migrate from pg2 to pg (OTP 23) #47

alisinabh opened this issue Jun 30, 2020 · 12 comments

Comments

@alisinabh
Copy link

Hi,

As of OTP 23, The pg2 module has been deprecated and replaced with similar but faster pg module.

I think for most parts changing pg2 with pg will do the trick.

But there are couple places in this library which used create/1 and delete/1 functions.
These functions are not present in pg.

All create/1 calls can be deleted.

Groups are automatically created when any process joins, and are removed when all processes leave the group. 
Non-existing group is considered empty (containing no processes).

source: https://erlang.org/doc/man/pg.html

But delete/1 is an issue! Since it may be used to clear a group but pg does not have a simple way of doing that.
A substitute would be:

pg:leave(Group, pg:get_members(Group))

I don't know if doing this is necessary!

And I wanted to thank you for your amazing libraries. ❤️

@alisinabh
Copy link
Author

Another big problem is

It is only allowed to join processes running on local node.

Seems like it also applies to leave/2 too. 😢
One cannot call leave on a remote pid.

@cabol
Copy link
Owner

cabol commented Jul 1, 2020

Hey, @alisinabh thank you very much first of all 😄 !

On the other hand, yes, migrating from pg2 to pg is something I had in mind too, and what I'm thinking to do initially is to move the distributed implementation shards_dist to a different repository, and make its usage optional. In this way, it will be easier to make a decision about how the distributed part should be. Also, it will be easier to evolve shards and shards_dist by separate. So, very soon I will publish the 0.7 release, with significant improvements and moving shards_dist out of it. And then, on the shards_dist repo we can provide the pg2 option and also another one using the new pg, but let's see, we definitely have to think about what is the best way 😉 !

Let me know your thoughts, stay tuned!

@alisinabh
Copy link
Author

Hi,

Thank you for your fast response.

I think separating shards_dist is a good idea. And it should support both OTP >= 23 and OTP < 23 like Phoenix.PubSub does (I think by checking existence of pg module during compile) .

@hmatinho
Copy link

hmatinho commented Sep 2, 2020

Any roadmap on the 0.7 release ? Thanks

@alisinabh
Copy link
Author

Hi @cabol,

I just wanted to say I would be happy to help on this one if you are busy.
If you want to, Just give me instructions and I will start.

Thanks again.

@cabol
Copy link
Owner

cabol commented Sep 20, 2020

Hey @alisinabh, yes sorry, I haven't had time for this one, but it would be awesome you can contribute.

I'm thinking of move the shards_dist module to a separate repository, this will make much easier evolving them. But anyways, this will take more time. In the meantime, we can address the PG issue. I'd create a module shards_cluster to encapsulate the used PG2/PG functions and make shards_dist agnostic to it, just using shards_cluster. And within shards_cluster, by means of ifdef, validate the current Erlang/OTP version, and if it is 23 or higher, it will use the pg module, otherwise, the pg2 will be used.

Maybe something like:

-module(shards_cluster).

-export([...]).

-ifndef(OTP_RELEASE).
  %% Because OTP_RELEASE macro was introduced since OTP 21, so ensure it is defined
  -define(OTP_RELEASE, 20). %=> I think it can be any value less than 21
-endif.

-if(?OTP_RELEASE >= 23).
  %% function impl using new pg
-else.
  %% use pg2
-endif.

I'd also validate the pg2 and pg functions, if the interface is the same, perhaps the only thing we have to do in the code above just to define the right PG module (-define(PG, pg) or -define(PG, pg2)).

I hope this could be helpful, otherwise, let me know, I'm glad to help!

Thanks!

@alisinabh
Copy link
Author

@cabol It's my pleasure, I'm on it. Will inform you of the progress...

@alisinabh
Copy link
Author

OK, So there is a problem. In the new pg module you can only join/leave a process to a group from the node it is running in!

So we cannot join or leave remote processes!

Another problem is the new pg module does not support delete/1 function. So we cannot purge a group with calling delete/1 which currently we use in our rename/2 function.

For the first problem we can use spawn/4 with node/1 to run the pg:join/2 or pg:leave/2 on the target node.

For the second problem we can again use spawn/4 on all connected nodes and call pg:leave/2 with the result of pg:get_local_members/1 on each node so we purge the process group.

Also create/1 is removed but I think just returning an ok atom will do the trick in this case.

@cabol Let me know what you think.

@cabol
Copy link
Owner

cabol commented Sep 20, 2020

Sounds good, I'm aware pg has a different implementation, so the idea is to find the proper solution for each case, so I think it should be fine. I'll check out pg impl details as soon as I can, but in the meantime, the suggestion makes a lot of sense (perhaps we can use shards_task).

@cabol
Copy link
Owner

cabol commented Oct 12, 2020

Hey @alisinabh!

There are several and new breaking changes in the master branch already (it should fix this issue). In short, shards was completely refactored, with a new and much better design, several improvements, etc., but they will be available since v1.0.0. Frum user/client standpoint there are no changes because shards in the end implement the same ETS API and that won't change. But, shards_dist was removed and the plan is to create a separate project for it, the issue is for the moment is not supported, since it has to be re-implement it according to the new architecture; hopefully soon 🤞 .

I created the repo for shards_dist, perhaps we can take it from there, create some GH issues for implementing the v1. Let me know your thoughts, stay tuned, THANKS!!

@alisinabh
Copy link
Author

Thank you very much for your efforts. I'm waiting for those issues.

Have a great day. ❤️

@cabol
Copy link
Owner

cabol commented Oct 28, 2020

I'll close this issue since it doesn't apply anymore, with shards v1 the distributed module shards_dist will be in a separate repo, so by the time there is a first implementation ready, it should provide support for both, pg and pg2 (for OTP < 23).

@cabol cabol closed this as completed Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants