Skip to content

Removing possibly outdated code from TrellServer.cs #43

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

doug-jacob
Copy link
Contributor

@doug-jacob doug-jacob commented Feb 12, 2025

Following a conversation with Matthew during recent Trell cleanup, there was one notable section of commented-out code which we aren't sure is still relevant, so its removal is its own PR

EDIT: the purpose of the entire method was in question, so this PR changed to evaluate it wholistically

@isaksky
Copy link
Member

isaksky commented Feb 12, 2025

I think we'd need to do the work first to see if that is needed. E.g., how are workers logging now - is that ok, or should it be centralized?

@doug-jacob doug-jacob changed the title Removing commented-out code from TrellServer.cs Removing possibly outdated code from TrellServer.cs Feb 12, 2025
@@ -31,9 +31,6 @@ service TrellServer {

// -------------------- START INTERNAL API -----------------------------

// Called from the worker (on a user's behalf) to log.
rpc Log(LogRequest) returns (google.protobuf.Empty);
Copy link
Contributor

Choose a reason for hiding this comment

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

If it does make sense to remove Log, it will make sense to remove LogRequest and any other messages/types it depends on.

@oconnor0
Copy link
Contributor

Structurally, each worker process loads the implementation of ITrellLogger defined in the config and logs via that. This direct logging makes sense, conceptually, to me, as it allows the plugin to define how logging is done - stdout, file, Redis, etc. but I don't know if it's working OK in the places it's being used.

@isaksky
Copy link
Member

isaksky commented Feb 12, 2025

This direct logging makes sense, conceptually, to me, as it allows the plugin to define how logging is done

Well the plugin could be loaded anywhere, so that doesn't seem like a difference in capability to me.

One downside of the way we are doing it now is that the plugin may use scarce resources, such like connections, so it doesn't scale as well. E.g., a setup running 50 workers now needs 50 connections/connection-pools to redis or postgres (for example), when maybe 1 would do.

Also the overhead of constantly starting/stopping those.

@oconnor0
Copy link
Contributor

We could define a LogViaServer : ITrellLogger implementation that uses the rpc Log on the server to log from the worker and then provide a way to specify whether the worker should load that logger or the same one as the server.

Though maybe that's unnecessary complexity/work at this point.

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.

3 participants