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

Change Queue on to Deque in InMemoryRequestJournalStore #2299

Merged
merged 11 commits into from
Aug 23, 2023

Conversation

pks-1981
Copy link
Contributor

@pks-1981 pks-1981 commented Aug 4, 2023

Change Queue on to Deque in InMemoryRequestJournalStore and ServeEvent

#2296

Submitter checklist

  • The PR request is well described and justified, including the body and the references
  • The PR title represents the desired changelog entry
  • The repository's code style is followed (see the contributing guide)
  • Test coverage that demonstrates that the change works as expected
  • For new features, there's necessary documentation in this pull request or in a subsequent PR to wiremock.org

@pks-1981 pks-1981 requested a review from a team as a code owner August 4, 2023 22:21
@pks-1981
Copy link
Contributor Author

pks-1981 commented Aug 5, 2023

@tomakehurst @oleg-nenashev
please look MR how it will be possible

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

I would definitely more confident about such a change if we had some performance test to prove impact when recording the journal and in the getter. My gut feeling is that it is a positive improvement, so I am +1 for trying it out in the beta

@oleg-nenashev oleg-nenashev added the needs-tom Tom's Train Project :) label Aug 8, 2023
@pks-1981
Copy link
Contributor Author

@tomakehurst @oleg-nenashev
MR finished. please look how it will be possible.

@pks-1981 pks-1981 changed the title Change Queue on to Deque in InMemoryRequestJournalStore and ServeEvent Change Queue on to Deque in InMemoryRequestJournalStore Aug 16, 2023
@oleg-nenashev oleg-nenashev removed the breaking Breaking change label Aug 22, 2023
@@ -407,7 +406,7 @@ public void doesNotDuplicateCookieHeaders() {
testClient.get("/duplicate/cookies", withHeader("Cookie", "session=1234"));

LoggedRequest lastRequest =
getLast(target.find(getRequestedFor(urlEqualTo("/duplicate/cookies"))));
getFirst(target.find(getRequestedFor(urlEqualTo("/duplicate/cookies"))), null);
Copy link
Member

Choose a reason for hiding this comment

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

I think this will be a breaking change for a lot of folks. How about we modify AbstractRequestJournal.getRequestsMatching(...) to reverse the results so that they'll come back in the same order as previously.

Copy link
Contributor Author

@pks-1981 pks-1981 Aug 23, 2023

Choose a reason for hiding this comment

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

@tomakehurst
But we will return to what we left...
Breaking changes it's normal when update version IMHO...
And as a user, I'm waiting in "history" for more recent entries to be at the top...

Copy link
Member

Choose a reason for hiding this comment

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

I mean reverse the result of the find query rather than the whole list, which would have a pretty trivial impact by comparison and only affect finding vs. fetching the whole journal.

Agree it's in principle acceptable to create breaking changes, but this one is avoidable and my instinct is that it'll annoy people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tomakehurst
Something like that:

    List<LoggedRequest> loggedRequests = getRequests().filter(thatMatch(requestPattern, customMatchers)).collect(toList());
    Collections.reverse(loggedRequests);
    return loggedRequests;

?

@tomakehurst tomakehurst merged commit 78f04e2 into wiremock:master Aug 23, 2023
6 of 7 checks passed
@tomakehurst
Copy link
Member

Thanks!

@pks-1981 pks-1981 deleted the 2296 branch August 24, 2023 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs-tom Tom's Train Project :)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants