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

Cleanup: Move AsyncEngine to libbase.la #1548

Closed
wants to merge 1 commit into from

Conversation

yadij
Copy link
Contributor

@yadij yadij commented Oct 27, 2023

No logic changes.

@rousskov rousskov self-requested a review October 27, 2023 13:09
Copy link
Contributor

@kinkie kinkie left a comment

Choose a reason for hiding this comment

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

LGTM

@yadij yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Oct 31, 2023
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

I do not think AsyncEngine belongs to libbase. It is small but not a "fundamental commonly-used piece of code" that libbase is meant for. Let's not move AsyncEngine.h for now.

I support removing the evidently unnecessary src/AsyncEngine.cc.

N.B. AsyncEngine is not closely related to AsyncCall and related classes that currently reside in libbase. And even those classes may not really belong to libbase. Moving them (and AsyncEngine?) into a new dedicated libasync or similar library may be a good idea, as was suspected from the very libbase beginning (commit 5bd7a21) when we did not have as many asynchronous calls or as much experience with them.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 31, 2023
@yadij
Copy link
Contributor Author

yadij commented Nov 6, 2023

I do not think AsyncEngine belongs to libbase. It is small but not a "fundamental commonly-used piece of code" that libbase is meant for. Let's not move AsyncEngine.h for now.

Correct reference for src/base/ content scope https://wiki.squid-cache.org/Features/SourceLayout. This file falls under the "has nowhere else to go". The existence of a "core" library was vetoed over a decade ago.

I support removing the evidently unnecessary src/AsyncEngine.cc.

N.B. AsyncEngine is not closely related to AsyncCall and related classes that currently reside in libbase.

Indeed. Nothing in libbase has any implicit relationship with anything else there.

@rousskov
Copy link
Contributor

rousskov commented Nov 6, 2023

I do not think AsyncEngine belongs to libbase. It is small but not a "fundamental commonly-used piece of code" that libbase is meant for. Let's not move AsyncEngine.h for now.

Correct reference for src/base/ content scope https://wiki.squid-cache.org/Features/SourceLayout.

I disagree because SourceLayout page is a collection of (often unvetted) opinions that do not necessarily represent Project consensus.

This file falls under the "has nowhere else to go".

libbase is not a collection of things that have nowhere else to go. Even SourceLayout, with all its problems, does not make this mistake in defining libbase scope!

@rousskov
Copy link
Contributor

rousskov commented Nov 6, 2023

Nothing in libbase has any implicit relationship with anything else there.

Some libbase elements are related (implicitly or explicitly). For example, eight Async*.h files are related to each other and to JobWait classes (also in libbase). There are other related things in libbase as well. However, these existing relationships are not relevant to the discussion about AsyncEngine. I only mentioned Async* classes in this context because they start with the same word as AsyncEngine which may lead to an (incorrect) conclusion that the two concepts are closely related and, hence, may belong to the same library. I am glad we agree that there is no such relationship.

@kinkie
Copy link
Contributor

kinkie commented Jan 7, 2024

In the interest of moving this forward, how about setting up a src/events subfolder (and related libevents library) async look and events related code? @yadij @rousskov

@rousskov
Copy link
Contributor

rousskov commented Jan 8, 2024

how about setting up a src/events subfolder (and related libevents library) async look and events related code?

Not sure what you meant by "async look".

If we want to group async calls- and async job-related code (currently mostly in src/base/) and events code (currently in src/) together, then I would recommend moving all that code into src/async/ rather than into src/events/: There is a lot more code/APIs to async calls/jobs than there is to events (and the events support itself is increasingly built on top of async calls)...

If we want to group just events-related code (currently in src/) together, then moving into src/events/ is fine. I am not sure there is enough events-related code (without async calls/jobs code) to designate a whole directory/library for it, but I would not be violently opposed to such a move if others are sure it is a good idea.

@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jan 9, 2024
@yadij yadij closed this Jan 25, 2024
@yadij yadij deleted the arc-libsquid-everything-1 branch January 25, 2024 14:13
@squid-anubis squid-anubis added the M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants