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

The second release of mod_mam #151

Merged
merged 39 commits into from
Jun 4, 2014
Merged

Conversation

arcusfelis
Copy link
Contributor

Changelog:

  • Add mod_mam_con_ca_arch and mod_mam_muc_ca_arch;
  • Fix error handling;
  • Add hook filter_local_packet;
  • Add mim_process_type into process dictionary

Tests are here

…berd_users_muc.localhost',{<<alicesroom>>,<<muc.localhost>>}],[]} during sending a MUC message.
Added an empty stop_server function.
Special entries were added into process dictionary:
* mim_host - host of the process;
* mim_process_type - category of the process.

These tags can be used for debugging, better introspection and logging.
Entries should not be used in business logic and/or from inside of the process.
@@ -272,6 +272,8 @@ do_recv(LogFun, RecvPid, SeqNum) when is_function(LogFun); LogFun == undefined,
%% Returns : void() | does not return
%%--------------------------------------------------------------------
init(Host, Port, User, Password, Database, LogFun, Parent) ->
put(mim_host, Host),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Host is not a MongooseIM host, but MySQL server's IP.

@michalwski
Copy link
Contributor

There is a lot of mod_mam_* files right now. What do you think of moving them do a dedicated subdirectory? Or maybe even other repo which will be a dependency for MongooseIM?

Fixed specs

Conflicts:
	apps/ejabberd/src/mod_mam.erl
	apps/ejabberd/src/mod_mam_cache_user.erl
	apps/ejabberd/src/mod_mam_muc.erl
	apps/ejabberd/src/mod_mam_muc_odbc_arch.erl
	apps/ejabberd/src/mod_mam_odbc_arch.erl
	apps/ejabberd/src/mod_mam_utils.erl
	rebar.config
	rel/reltool.config
While Base 36 is slightly shorter, Base 32 is easy to read
@michalwski
Copy link
Contributor

@arcusfelis, could you rebase or merge with master?
And what do you think about my prev comment (regarding moving mod_mam_* file to dedicated subdirectory)?

@arcusfelis
Copy link
Contributor Author

Fine, I will rebase or merge.
I do not like the idea of changing file names, because it makes working with git history harder.

@michalwski
Copy link
Contributor

I do not like the idea of changing file names, because it makes working with git history harder.

Git is one thing, but clean code is another thing. Let's put that on vote. Could everyone involved in the project say wether it is good idea to move all mod_mam_* module to dedicated subdirectory or is it better to keep them as is?

@michalwski
Copy link
Contributor

And here goes my vote:
👍 for dedicated directory

@fenek
Copy link
Member

fenek commented Jun 2, 2014

👍 for dedicated directory, also 👍 for dedicated directory for mod_admin_extra_*.erl - it also has many files.

@pzel
Copy link
Contributor

pzel commented Jun 3, 2014

☝︎ for dedicated dir. Also, it might make sense to move the larger modules into their own apps/mod* directories. (This is a separate topic, but please think about it)

@erszcz
Copy link
Member

erszcz commented Jun 3, 2014

I hold from voting, but would like to pinpoint that introducing extra dirs per module is repeating history. In pre-fork ejabberd there were actually multiple directories below src/ (e.g. odbc, web, pubsub). One of the steps performed when making MongooseIM (then just esl-ejabberd) OTP/Rebar compliant was flattening that layout.
Is introducing subdirs to src/ even compliant with Rebar?

@pzel
Copy link
Contributor

pzel commented Jun 3, 2014

@Lavrin: and the apps/your_module_here approach? Would it help here?

@michalwski
Copy link
Contributor

Is introducing subdirs to src/ even compliant with Rebar?

Yes, it is.

Also, it might make sense to move the larger modules into their own apps/mod* directories. (This is a separate topic, but please think about it)

I think that's better solution. This approach has additional advantage - we can easily add/remove such app from Erlang release making the final release smaller if given big functionality is not required. Good candidates for that are mod_mam and, as @fenek noticed, mod_admin_extra.

michalwski added a commit that referenced this pull request Jun 4, 2014
@michalwski michalwski merged commit b4c7930 into esl:master Jun 4, 2014
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

Successfully merging this pull request may close these issues.

5 participants