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

ISSUE-1619: Implement session flow control using new Proton window API #1647

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

Conversation

kgiusti
Copy link
Contributor

@kgiusti kgiusti commented Oct 23, 2024

No description provided.

// amount of outgoing data that can be buffered on a session before control is returned to Proton. This is necessary to
// improve latency and allow capacity sharing among all links on the session.
//
const size_t qd_session_max_outgoing_bytes = 1048576; // max buffered bytes on a session
Copy link
Contributor

@ganeshmurthy ganeshmurthy Oct 23, 2024

Choose a reason for hiding this comment

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

Suggested change
const size_t qd_session_max_outgoing_bytes = 1048576; // max buffered bytes on a session
const size_t qd_session_max_outgoing_bytes = 1048576; // 1 MB is the maximum bytes that can be written to an outgoing session before backing off.

// improve latency and allow capacity sharing among all links on the session.
//
const size_t qd_session_max_outgoing_bytes = 1048576; // max buffered bytes on a session
const size_t qd_session_low_outgoing_bytes = 524288; // low watermark for max buffered bytes
Copy link
Contributor

@ganeshmurthy ganeshmurthy Oct 23, 2024

Choose a reason for hiding this comment

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

Suggested change
const size_t qd_session_low_outgoing_bytes = 524288; // low watermark for max buffered bytes
const size_t qd_session_low_outgoing_bytes = 524288; // Start writing outgoing bytes to a session only if the session outgoing capacity is greater than 0.5 MB. low watermark for max buffered bytes

size_t out_window_limit;
size_t out_window_low_watermark;
};
const size_t qd_session_max_in_win_user = (size_t) 8388608; // AMQP application in window max bytes 8MB
Copy link
Contributor

Choose a reason for hiding this comment

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

AMQP application

It is not clear to me what AMQP application means

size_t out_window_low_watermark;
};
const size_t qd_session_max_in_win_user = (size_t) 8388608; // AMQP application in window max bytes 8MB
const size_t qd_session_max_in_win_trunk = (size_t) 134217728; // inter-router in window max bytes 128MB
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
const size_t qd_session_max_in_win_trunk = (size_t) 134217728; // inter-router in window max bytes 128MB
const size_t qd_session_max_in_win_trunk = (size_t) 134217728; // per inter-router connection window max byte limit 128MB

Comment on lines +659 to 671
// Assume defaults will be used
*in_window = cf->session_max_in_window;

if (qd_conn->policy_settings) {
const qd_policy_spec_t *spec = &qd_conn->policy_settings->spec;
if (!spec->outgoingConnection && spec->maxSessionWindow) {
// Policy configures the window *in bytes* but Proton uses *frames*. Convert to frames
uint32_t max_frame = spec->maxFrameSize ? spec->maxFrameSize : cf->max_frame_size;
*in_window = spec->maxSessionWindow / max_frame;
if (*in_window < 2)
*in_window = 2;
}
}
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
// Assume defaults will be used
*in_window = cf->session_max_in_window;
if (qd_conn->policy_settings) {
const qd_policy_spec_t *spec = &qd_conn->policy_settings->spec;
if (!spec->outgoingConnection && spec->maxSessionWindow) {
// Policy configures the window *in bytes* but Proton uses *frames*. Convert to frames
uint32_t max_frame = spec->maxFrameSize ? spec->maxFrameSize : cf->max_frame_size;
*in_window = spec->maxSessionWindow / max_frame;
if (*in_window < 2)
*in_window = 2;
}
}
if (qd_conn->policy_settings) {
const qd_policy_spec_t *spec = &qd_conn->policy_settings->spec;
if (!spec->outgoingConnection && spec->maxSessionWindow) {
// Policy configures the window *in bytes* but Proton uses *frames*. Convert to frames
uint32_t max_frame = spec->maxFrameSize ? spec->maxFrameSize : cf->max_frame_size;
*in_window = spec->maxSessionWindow / max_frame;
if (*in_window < 2)
*in_window = 2;
} else {
*in_window = 2;
}
} else {
// Assume defaults will be used
*in_window = cf->session_max_in_window;
}


// Session windowing limits
extern const size_t qd_session_max_in_win_user; // incoming window byte limit for user connections
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
extern const size_t qd_session_max_in_win_user; // incoming window byte limit for user connections
extern const size_t qd_session_max_incoming_win_user; // incoming window byte limit for user connections


// Session windowing limits
extern const size_t qd_session_max_in_win_user; // incoming window byte limit for user connections
extern const size_t qd_session_max_in_win_trunk; // incoming window byte limit for inter-router connections
Copy link
Contributor

@ganeshmurthy ganeshmurthy Oct 23, 2024

Choose a reason for hiding this comment

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

Suggested change
extern const size_t qd_session_max_in_win_trunk; // incoming window byte limit for inter-router connections
extern const size_t qd_session_max_incoming_conn_win; // incoming window byte limit for inter-router connections

@@ -69,15 +69,34 @@ struct qd_session_t {
sys_atomic_t ref_count;
pn_session_t *pn_session;
qd_link_list_t q3_blocked_links; ///< Q3 blocked if !empty
uint32_t remote_max_frame;
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
uint32_t remote_max_frame;
uint32_t remote_max_frame_size;

// Proton does not support maxSessions > 32768
int64_t value = (int64_t) qd_entity_get_long(entity, "maxSessions"); CHECK();
if (value == 0) {
value = 32768; // default
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
value = 32768; // default
value = QD_PN_MAX_SESSIONS; // default

define QD_PN_MAX_SESSIONS in amqp.h ?

// and it does not exceed the proton APIs max of INT32_MAX
value = (int64_t) qd_entity_get_long(entity, "maxFrameSize"); CHECK();
if (value == 0) {
value = 16384; // default
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
value = 16384; // default
value = QD_DEFAULT_MAX_FRAME_SIZE; // default

*/
size_t incoming_capacity;
uint32_t session_max_in_window;
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
uint32_t session_max_in_window;
uint32_t session_max_incoming_window;

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