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

Add logging for ChunkMeta #387

Merged
merged 6 commits into from
Mar 25, 2024
Merged

Add logging for ChunkMeta #387

merged 6 commits into from
Mar 25, 2024

Conversation

okx-code
Copy link
Contributor

Hopefully this can be used to diagnose the issue

return ENABLE_CHUNK_META_LOGS;
}

@Subcommand("chunkmeta")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is intuitive as a toggle, the command should be something like /cmc togglechunkmetalogs or /cmc chunkmetalogs true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change that

@CommandAlias("cmc")
public class ChunkMetaCommand extends BaseCommand {

private static boolean ENABLE_CHUNK_META_LOGS = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

The command shouldn't hold what's effectively global state itself, there should be some dedicated collection of feature flags be it the config, or secondarily derived from the config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it shouldn't but it was the easiest option for a very accessory feature

Comment on lines 231 to 233
if (ChunkMetaCommand.chunkMetaLogsEnabled()) {
logger.info("[Chunkmeta] Unloading chunk " + coord + " - unloaded: " + coord.getLastUnloadedTime());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be logging with guard conditions like this in modern java, we should be using the appropriate log level and a message supplier to prevent resources spent on extra checks and calculations.

This also makes the above command unnecessary.

e.g.

logger.debug(() -> "[Chunkmeta] Unloading chunk " + coord + " - unloaded: " + coord.getLastUnloadedTime())

Copy link
Contributor Author

@okx-code okx-code Mar 23, 2024

Choose a reason for hiding this comment

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

I did some research into this and there's not really any good options for runtime log configuration like that, and since it's temporary I just went for something easy

Copy link
Contributor

badge

Build Successful! You can find a link to the downloadable artifact below.

Name Link
Commit da67fa4
Logs https://github.com/CivMC/Civ/actions/runs/8427205373
Download https://github.com/CivMC/Civ/suites/22092677250/artifacts/

@okx-code okx-code merged commit bdfd6c2 into main Mar 25, 2024
2 checks passed
@okx-code okx-code deleted the meta-log branch March 25, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants