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

ARTEMIS-3327 Removing unecessary block operation on journal append record #3644

Closed
wants to merge 1 commit into from

Conversation

clebertsuconic
Copy link
Contributor

No description provided.

@clebertsuconic
Copy link
Contributor Author

this is simply an optimization. There are no semantic changes on this commit.

@jbertram
Copy link
Contributor

jbertram commented Jul 6, 2021

My reading here is that the result wasn't actually used for anything so there was no reason to keep it. Is that how you see it?

@clebertsuconic clebertsuconic changed the title ARTEMIS-3327 Removing unecessary block oepration on journal append record ARTEMIS-3327 Removing unecessary block operation on journal append record Jul 6, 2021
@clebertsuconic
Copy link
Contributor Author

@jbertram that's right

@asfgit asfgit closed this in d800957 Jul 6, 2021
@brusdev
Copy link
Member

brusdev commented Jul 7, 2021

@clebertsuconic was the result used to wait the appendExecutor execution?

@gemmellr
Copy link
Member

gemmellr commented Jul 7, 2021

Depends on the provided values of 'sync' and 'callback'. The called newSyncAndCallbackResult(sync, callback) method creating the future did this:
return (sync && callback == null) ? new SimpleFutureImpl<>() : SimpleFuture.dumb();

If asked to sync and there was no callback, it returns a 'proper future' that could be used to actually wait for the execution, and detect failures from thrown exceptions passed via the fail method.

If not asked to sync, or a callback is given, it returns a 'dumb' future that doesnt wait at all and just returns null immediately, ignores completions/failures etc.

I dont know what the sync and callback values actually are for this situation but I guess from that they were eitehr always false or non-null. Might be worth adding a check to that effect though.

@clebertsuconic
Copy link
Contributor Author

@gemmellr / @brusdev / @jbertram I was thinking only about the artemis use case here, where we always use this method with sync=false.

I know some people is using the journal directly (who actually requested me to make the journal an independent project a few times).. and they actually will use the sync version.

I will just revert this method. thanks for the comments.

@gemmellr
Copy link
Member

gemmellr commented Jul 7, 2021

Might be worth adding a comment, and test, to avoid a recurrence.

@clebertsuconic clebertsuconic deleted the ARTEMIS-3327 branch July 7, 2021 22:23
@gemmellr
Copy link
Member

gemmellr commented Jul 8, 2021

Revert and test addition were done via #3647

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.

4 participants