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

Datagram drop callback #2100

Merged
merged 7 commits into from
Dec 30, 2022

Conversation

Wisien999
Copy link

Discussion ref: #2099

I'd like to introduce injectable action on datagram/record drop with access to dropped object. As discussed in the issue two methods were prepared to be added to DatagramFilter.

@@ -1969,7 +2004,7 @@ public void run() {
* @since 3.5
*/
@NoPublicAPI
protected boolean executeInbound(Executor executor, InetSocketAddress peer, LimitedRunnable job) {
protected boolean executeInbound(Executor executor, InetSocketAddress peer, LimitedRunnable job, Runnable onError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, extending LimitedRunnablewith an onError(RejectedExecutionException ex) callback is an alternative to a second Runnable.
Or pass in the Record instead of the second Runnable

@@ -0,0 +1,40 @@
/*******************************************************************************
* Copyright (c) 2022 Bosch.IO GmbH and others.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you an employee of Bosch.IO?

If not, please edit this according license_header_template.txt. if this is your own work not belonging to an employer, then just use your name.

There is also a pending PR #2097 . If you like, you may chose also that. I plan to replace all header when start to work in the 4.0.

@boaks
Copy link
Contributor

boaks commented Dec 27, 2022

Thanks a lot!

I would like to spend some time in testing it.
Reviewing, I also recognized, that there is the one or other "gap" related to the "running".
Please correct the license header in the DatagramFilterExtended.

@boaks
Copy link
Contributor

boaks commented Dec 27, 2022

I see two more candidates, dropReceivedRecord(Record record) and discardRecord(final Record record, final Throwable cause).

/**
* Handles {@code RejectedExecutionException}
* @param ex the thrown exception
*/
Copy link
Contributor

@boaks boaks Dec 27, 2022

Choose a reason for hiding this comment

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

Some polishing, please add

@since 3.8


/**
* Extension of DatagramFilter
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Some polishing, please add a "." at the end of the first sentence and a

@since 3.8

@Wisien999
Copy link
Author

Wisien999 commented Dec 28, 2022

Thank you for noticing dropReceivedRecord and discardRecord I overlooked. I also added the callback to the end of processApplicationDataRecord(final Record record, final Connection connection) where health.receivingRecord(true) is not called. If you think it should not be here I can remove it.

@boaks
Copy link
Contributor

boaks commented Dec 29, 2022

LGTM

@boaks boaks merged commit 41c6cb8 into eclipse-californium:main Dec 30, 2022
@boaks
Copy link
Contributor

boaks commented Dec 30, 2022

I missed to ask for squashing.
I will just do this on my own ;-).

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