Skip to content

Conversation

@reddyashish
Copy link
Contributor

Purpose

This is to test Dynamo player with 2.19.2 version. It is to skip the lucene indexing for DynamoPlayer and GD.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

SkipLuceneindexing flag for DynamoPlayer

Reviewers

@QilongTang @BogdanZavu

@github-actions
Copy link

⚠️ [run-bin-diff] - Files Added/Deleted::2 file(s) have been deleted!
⚠️ [run-bin-diff-net60-windows] - Files Added/Deleted::2 file(s) have been deleted!

@BogdanZavu
Copy link
Contributor

BogdanZavu commented Sep 28, 2023

I believe we need a few more changes in D4R repo - that is we need some journal key there that will passed on as skip lucene in the startup config here. That is so that we can stop the indexing from DynamoPlayer context.
Or actually we can turn off the searching directly in D4R code which will apply to Player as well without the need to add extra journal keys there. It is off by default if we want to go this way.
GD Should not be affected but just in case if we want to stop this in CLI mode as well we need to have a new CLI startup parameter as well.

But it looks like we can hold off with this fix in the near future and deal with the performance.

@QilongTang
Copy link
Contributor

@BogdanZavu @twastvedt With the recent performance improvement, do you think if we still need this change?

@QilongTang QilongTang added the DNM Do not merge. For PRs. label Oct 3, 2023
@twastvedt
Copy link
Contributor

The numbers you posted look really good. I'd say no, at least for now.

@BogdanZavu
Copy link
Contributor

I think we can hold off on this change for now.
But perhaps we can come back to it later. Maybe we don't even need a flag, is there any reason to do the indexing in headless/clis modes ?

@QilongTang
Copy link
Contributor

@BogdanZavu @twastvedt Thank you for confirmation. But I think indexing is not strictly required especially in service mode.

@twastvedt
Copy link
Contributor

Yep, definitely agree that ideally we'd be able to turn off indexing. Maybe always in both CLI and service modes? I agree with Bogdan that we don't need a flag, just don't have it on where it's not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DNM Do not merge. For PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants