Civcraft / NameLayer

Do not open issues here; open them on the maintained fork @ DevotedMC
https://github.com/DevotedMC/NameLayer
BSD 3-Clause "New" or "Revised" License
4 stars 14 forks source link

Async & Mercury Changes #212

Closed suirad closed 8 years ago

suirad commented 8 years ago
CivcraftBot commented 8 years ago

Can one of the admins verify this patch? Type 'ok to test' to test.

Maxopoly commented 8 years ago

Looks good overall, but I think it would be a lot better if all mercury messages sent were together in one MercuryManager class. Right now they are spread out all over the place and that makes debugging them very hard.

suirad commented 8 years ago

You should elaborate on that suggestion. The mercury parts aren't much different then how they were previously. Is that something that should delay this pull?

Maxopoly commented 8 years ago

So my concern with the mercury messages is just that its very hard to debug them if they are all over the place. For normal function I can easily look up where they are called, but for Mercury messages I cant, so having all messages being sent in a single class makes things a lot easier and cleaner imo. If Dan disagrees with this opinion, just go ahead and ignore me

ProgrammerDan commented 8 years ago

Ok, I've reviewed. Sadly this PR is not ready for inclusion. Some of the issues are Namelayer's and not the fault of this Pull, and if it were those issues alone I would merge it now. However, some other issues do remain with this PR itself, and those need to be resolved or clarified before we can proceed.

suirad commented 8 years ago

Added another commit, correcting review above. Please review.

ttk2 commented 8 years ago

bump, whats the status of this?

ttk2 commented 8 years ago

soo shards are crashing and we need these fixes, progress?

ProgrammerDan commented 8 years ago

i'm a bit gun-shy after our last batch of async changes caused enormous duplication problems....

was namelayer the cause of the crash?

Maxopoly commented 8 years ago

Yes, group creation took too long

suirad commented 8 years ago

Well after our discussions Dan, most of my async changes add capability, but are not in use. As in it will have the capability if we find good places that need it, but it's not being used anywhere in the codebase. Merging in its current state will primarily change how mercury messages are handled as each message modifies the cache directly instead of forcing recache from the db for every little change, across every shard.

Really the only thing left for this is a db upgrade to strip pipe chars from group names. I won't have time to hack on it till tomorrow, unless someone else wants wants to take the helm I'd appreciate it ;).

On Sat, Aug 20, 2016, 10:21 PM Maxopoly notifications@github.com wrote:

Yes, group creation took too long

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Civcraft/NameLayer/pull/212#issuecomment-241236657, or mute the thread https://github.com/notifications/unsubscribe-auth/AIZCbdNOxH7s2DRNE7wjZZFD4Ji8zfByks5qh8QkgaJpZM4JeaYL .

ttk2 commented 8 years ago

Is the encoding ASSCI? if its Unicode then I think we could find a much better char than | to use.

But I don't care that much, even if we break existing groups with | in them although an upgrade path that strips all group names of | would be great.

ProgrammerDan commented 8 years ago

Something went wrong here

ProgrammerDan commented 8 years ago

Note to future: use git rebase -i master where master is a clean checkout of remote civcraft's master branch. This will recreate your commit nodes, yes, but cleanly; so that history is preserved as linear and more useful then the commitspam that happens if you begin to multiply merge master into a branch that was created some time ago.

Or better as ttk2 suggested use git rebase -i origin/master assuming origin points to this repository :)