Skip to content

Conversation

@ywkaras
Copy link
Contributor

@ywkaras ywkaras commented Aug 18, 2022

Getting a result from it, without blocking event thread.

@ywkaras ywkaras self-assigned this Aug 18, 2022
@ywkaras ywkaras added this to the 10.0.0 milestone Aug 18, 2022
NOTICE Outdated
~~~

lib/cppapi developed by LinkedIn
include/tscpp/api developed by LinkedIn
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes are unrelated cleanup, from moving lib/cppapi to include/tscpp/api and src/tscpp/api .

@ywkaras ywkaras force-pushed the polite_hook_wait branch 2 times, most recently from 46a46c5 to f274fad Compare August 18, 2022 01:21
@ywkaras ywkaras marked this pull request as draft August 18, 2022 14:08
@ywkaras ywkaras force-pushed the polite_hook_wait branch 2 times, most recently from 5125e1f to aeb8791 Compare August 18, 2022 14:58
@ywkaras ywkaras marked this pull request as ready for review August 18, 2022 15:55
@maskit
Copy link
Member

maskit commented Aug 22, 2022

API review on dev@ ?

@ywkaras
Copy link
Contributor Author

ywkaras commented Aug 22, 2022

API review on dev@ ?

There is not change to the TS API.

@ywkaras ywkaras requested a review from rob05c August 22, 2022 14:45
~~~

lib/cppapi developed by LinkedIn
include/tscpp/api, src/tscpp/api developed by LinkedIn
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This included some unrelated cleanup of kruft leftover from lib/cppapi moving to include|src/tscpp/api .

@maskit
Copy link
Member

maskit commented Aug 22, 2022

API review on dev@ ?

There is not change to the TS API.

plugins/xdebug/Cleanup.h → include/tscpp/api/Cleanup.h

You are introducing a new cpp api. See discussion on #6197.

@ywkaras
Copy link
Contributor Author

ywkaras commented Aug 22, 2022

API review on dev@ ?

There is not change to the TS API.

plugins/xdebug/Cleanup.h → include/tscpp/api/Cleanup.h

You are introducing a new cpp api. See discussion on #6197.

OK done.

@maskit
Copy link
Member

maskit commented Aug 29, 2022

@ywkaras I meant you should send API change proposal on dev@ and I thought you did it, but I couldn't find the mail. What was done? You mean you saw the discussion?

@ywkaras
Copy link
Contributor Author

ywkaras commented Aug 30, 2022 via email

@ywkaras
Copy link
Contributor Author

ywkaras commented Aug 30, 2022

MIME-Version: 1.0
Date: Mon, 22 Aug 2022 10:25:18 -0500
Reply-To: wkaras@oath.com
Message-ID: <CANMHUGSnbxdT5z+mPG7ZfTDoAG_bW3p77dMYHwb2jhoT_-7pSg@mail.gmail.com>
Subject: Proposed change to CPP API
From: Walt Karas <wkaras@yahooinc.com>
To: dev <dev@trafficserver.apache.org>
Content-Type: multipart/alternative; boundary="0000000000002acc7f05e6d60ebf"

--0000000000002acc7f05e6d60ebf
Content-Type: text/plain; charset="UTF-8"

I propose moving Cleanup.h from plugins/xdebug to include/ts/api as a part
of https://github.com/apache/trafficserver/pull/9044 .

--0000000000002acc7f05e6d60ebf
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">I propose moving Cleanup.h from plugins/xdebug to include/=
ts/api as a part of=C2=A0<a href=3D"https://github.com/apache/trafficserver=
/pull/9044">https://github.com/apache/trafficserver/pull/9044</a> .</div>

--0000000000002acc7f05e6d60ebf--

maskit
maskit previously requested changes Sep 6, 2022
Copy link
Member

@maskit maskit left a comment

Choose a reason for hiding this comment

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

Thanks for proposing the API change on the list. The change should be made separately even if it's accepted.

@ywkaras
Copy link
Contributor Author

ywkaras commented Sep 6, 2022

Thanks for proposing the API change on the list. The change should be made separately even if it's accepted.

How would I do that? I an using definitions in cleanup.h in the test's plugin.

@ywkaras ywkaras marked this pull request as draft September 6, 2022 15:59
@apache apache deleted a comment from bryancall Sep 6, 2022
Getting a result from it, without blocking event task.
@ywkaras ywkaras marked this pull request as ready for review September 20, 2022 00:40
@ywkaras
Copy link
Contributor Author

ywkaras commented Sep 20, 2022

OK @maskit the change you requested has been made.

@maskit maskit dismissed their stale review September 20, 2022 14:50

Requested change was made

@maskit
Copy link
Member

maskit commented Sep 20, 2022

I'll leave actual code review to @rob05c,

@ywkaras
Copy link
Contributor Author

ywkaras commented Nov 9, 2022

@rob05c should I find someone else to review this?

@ywkaras
Copy link
Contributor Author

ywkaras commented Dec 2, 2022

@rob05c will you have time to review this? Should we get someone else?

@bryancall bryancall requested review from maskit and removed request for rob05c December 13, 2022 00:18
@ywkaras ywkaras merged commit ac0c764 into apache:master Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants