Skip to content
This repository was archived by the owner on Feb 17, 2024. It is now read-only.

Conversation

@Diet-Cola
Copy link

Some of these are definitely my bad from lack of knowledge, more specifically the use of .autoCommit() however these fixes at the time massively overhauled the time taken to shutdown the server when CivClassics was taking 10+ mins per plugin to save out data on shutdown.

Thank you for the reviews and pull requests!

@psygate
Copy link
Author

psygate commented Jun 18, 2022

Test my changes thoroughly before putting them live. I'm a dumdum just like everybody else.
No shame in learning.

Copy link

@Aleksey-Terzi Aleksey-Terzi left a comment

Choose a reason for hiding this comment

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

Overall looks good.

A couple of things:

  1. Not sure about Timer class, looks like we will be getting too much spam in the log?

  2. Good work on batches, though maybe we don't need batches at all?
    Previously we didn't have a periodic data saving and therefore the plugin was saving all changes accumulated for the day.
    Though now we save data every 1 min (the async task in CivModCore) and the shutdown just saves data that wasn't saved in the last 1 min.
    At the same time, batches make code more complicated.
    Therefore I propose to delete the batching at all.

continue;

// Add to a list to avoid connection deadlocks.
List<Reinforcement> inserts = new ArrayList<>();

Choose a reason for hiding this comment

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

Not sure we need to create this list here.
The consumer function is pretty fast, this is what the consumer calls in the end (isNew = false):
https://github.com/CivMC/CivModCore/blob/master/paper/src/main/java/vg/civcraft/mc/civmodcore/world/locations/chunkmeta/block/BlockBasedChunkMeta.java#L227

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Status: Ready for Review

Development

Successfully merging this pull request may close these issues.

4 participants