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

Add Reverse ordering support for Task API #816

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ma1581
Copy link
Contributor

@ma1581 ma1581 commented Feb 9, 2025

Pull Request

Related issue

Fixes #798

What does this PR do?

  • Accepts 'reverse' field to provider ordering flexibility

PR checklist

Please check if your PR fulfills the following requirements:

  • [ Y ] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • [ Y ] Have you read the contributing guidelines?
  • [ Y ] Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@ma1581 ma1581 marked this pull request as ready for review February 9, 2025 16:09
@Strift
Copy link
Contributor

Strift commented Feb 17, 2025

Hi @ma1581, thank you for your PR. 🙌

I merged the changes from the main branch (related to your other PR.) But now, the tests are not passing. Can you look into it?

Thanks!

@ma1581
Copy link
Contributor Author

ma1581 commented Feb 22, 2025

Hi @Strift . thanks for notifying this.
Have updated the implementation and junit to match the time format. pls check it

Glad to be part of this

@Strift Strift requested a review from curquiza February 27, 2025 08:17
Copy link
Member

@brunoocasali brunoocasali left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your PR!

Next time I would ask you to make one change per PR, it makes it easier to review :)

Comment on lines +18 to +22
@BeforeEach
void beforeEach() {
TimeZone.setDefault(TimeZone.getTimeZone("UTC"));
}

Copy link
Member

Choose a reason for hiding this comment

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

Why did you need to set this? And if "yes" why not to set this for the whole test suite?

Comment on lines +221 to +225
Task[] defaultTaskList =
client.getTasks(new TasksQuery().setAfterEnqueuedAt(currentTime)).getResults();
Task[] reversedTaskList =
client.getTasks(new TasksQuery().setAfterEnqueuedAt(currentTime).setReverse(true))
.getResults();
Copy link
Member

Choose a reason for hiding this comment

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

Can you map only the task ids here, and then you assert them later, so the test will be less complex to read.

TaskInfo taskA = client.index(indexUid).addDocuments(testData.getRaw());
TaskInfo taskB = client.index(indexUid).addDocuments(testData.getRaw());

client.waitForTask(taskA.getTaskUid());
Copy link
Member

Choose a reason for hiding this comment

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

If you wait for the last task the first one will be finished in any case so you can remove this line.

Arrays.stream(defaultTaskList).map(Task::getUid).collect(Collectors.toList());
List<Integer> reversedTaskOrder =
Arrays.stream(reversedTaskList).map(Task::getUid).collect(Collectors.toList());
assertFalse(originalTaskOrder.isEmpty());
Copy link
Member

Choose a reason for hiding this comment

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

You can also remove one of these assertions.

@@ -202,6 +205,38 @@ public void testClientGetTasksAllQueryParameters() throws Exception {
assertThat(result.getNext(), is(notNullValue()));
assertThat(result.getResults().length, is(notNullValue()));
}
/** Test to check if task list is reversed when enabled */
Copy link
Member

Choose a reason for hiding this comment

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

No need to explain what you're testing, this is usually a smell that your test is too complex. Which is not the case :)

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.

[v1.12.0] Add reverse parameter for fetching tasks
3 participants