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 page on core developers conversations #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Jan 17, 2024

No description provided.

@kinkie kinkie requested review from rousskov and yadij January 17, 2024 11:38
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.

Thank you for starting this work. I believe the result of this effort can be very useful.

[PR 1635](https://github.com/squid-cache/squid/pull/1635) and
[PR 1548](https://github.com/squid-cache/squid/pull/1548).

* **[C]** in this conversation we are not concerned with the mechanisms by which individual calls are fired
Copy link
Contributor

Choose a reason for hiding this comment

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

Why say this? We are not concerned with a lot of things. What is so special about the firing mechanism? Does the firing mechanism include the concept of firing the call away from (and at a different time than) call creation? If it does, then there is no consensus regarding this statement.

Suggested change
* **[C]** in this conversation we are not concerned with the mechanisms by which individual calls are fired

[PR 1548](https://github.com/squid-cache/squid/pull/1548).

* **[C]** in this conversation we are not concerned with the mechanisms by which individual calls are fired
* **[C]** AsyncCall is a delayed function call, that can change the stack
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we describing what AsyncCall is? AsyncCall is an existing class. If our understanding of that class differs from that class description, we can document that, but that requires a rather different statement (and, ideally, we should just fix the class documentation instead).

[PR 1548](https://github.com/squid-cache/squid/pull/1548).

* **[C]** in this conversation we are not concerned with the mechanisms by which individual calls are fired
* **[C]** AsyncCall is a delayed function call, that can change the stack
Copy link
Contributor

@rousskov rousskov Jan 17, 2024

Choose a reason for hiding this comment

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

Does a C++ function pointer represent an asynchronous call concept as well? Like AsyncCall, a function pointer is used to delay the function call and, when used/fired/called, it (like any function call) "changes the stack". Either there is something missing in this attempt to define asynchronous call concept OR we should not tie that definition to the AsyncCall class so much.

* **[C]** in this conversation we are not concerned with the mechanisms by which individual calls are fired
* **[C]** AsyncCall is a delayed function call, that can change the stack
* **[C]** our strategic goal is that all call sthat cannot be expressed as a c++ function call be implemented as AsyncCalls, including all callbacks
* **[C]** call queues and lists and so on are logically tied to calls, not to the firing mechanisms
Copy link
Contributor

Choose a reason for hiding this comment

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

Some call collections may be tied to the firing mechanisms as well.

Suggested change
* **[C]** call queues and lists and so on are logically tied to calls, not to the firing mechanisms

Comment on lines +26 to +27
The proposal should include a high level idea about why things should be moved;
it does not need to include any description of why other things should not be moved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The proposal should include a high level idea about why things should be moved;
it does not need to include any description of why other things should not be moved
The proposal should define what the new directory should be used for
(while keeping in mind that a list of file names is not a good definition).

means the point has been discussed but no final decision has been made


## 2024-01-16
Copy link
Contributor

@rousskov rousskov Jan 17, 2024

Choose a reason for hiding this comment

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

IMHO, the current CoreDevConvos page intent (as implied by the above description) would be too costly to support and will become stale/abandoned. For example, it may take us several weeks to polish and merge [C] bullet points; by that time, even if we end up with something more useful than a couple of obvious facts, we will be a meeting behind. Also, meeting date is a poor choice for the primary key because establishing consensus often takes more than one meeting and often takes place outside of a meeting (e.g., this PR reviews); we should be focusing on concepts rather than their live discussion schedule.

Instead of a diary, we should maintain a collection of things we agree on (including, in some special cases, things we agree to disagree on). Git log and GitHub history can always be used to map that collection to a diary if anybody really needs the latter.

FWIW, I have proposed to add such a collection of notes and even bootstrapped it at https://github.com/measurement-factory/squid-notes/blob/start/README.md. I suggest refactoring the proposed docs/RoadMap/CoreDevConvos.md page as a page dedicated to asynchronous calls in Squid and moving this page into a new docs/notes directory (where other notes like that can be accumulated, debated, and maintained).

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.

2 participants