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

WIP: Decoupling Translog from Engine #3471

Closed
wants to merge 47 commits into from

Conversation

Bukhtawar
Copy link
Collaborator

Signed-off-by: Bukhtawar Khan [email protected]

Description

Addresses #3241, the initial draft of the changes, doesn't cover tests, demonstrates the basic structural changes involved

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Bukhtawar Khan <[email protected]>
@Bukhtawar Bukhtawar requested review from a team and reta as code owners May 30, 2022 09:28
@Bukhtawar Bukhtawar marked this pull request as draft May 30, 2022 09:28
@Bukhtawar Bukhtawar changed the title Decoupling Translog from Engine WIP: Decoupling Translog from Engine May 30, 2022
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure c977906069264f14325bfd4283349fe26808203c
Log 5677

Reports 5677

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure a79c8d6dd244e371b48bb47baf436d5eb59714eb
Log 5678

Reports 5678

Signed-off-by: Bukhtawar Khan <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 72dd53d
Log 5683

Reports 5683

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 4febe1a6fec5f42b0c57c4056b4c7d48911400eb
Log 5685

Reports 5685

Signed-off-by: Bukhtawar Khan <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 14187a7
Log 5686

Reports 5686

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 8fa5b7671aa67535f06a2e289f02351a3a235a56
Log 5704

Reports 5704

import java.util.stream.Stream;

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the crux of the decoupling, so a good comment here will be really helpful. I don't have any good suggestions beyond "orchestrates translog operations" or something generic like that, but a comment would be helpful to explain why this exists, what it is intended to do, how it relates to the Translog class, etc.

Also, can this be package private? And any reason to prefer an abstract class over an interface since no implementations are provided here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @andrross, yes the java docs are WIP, will fix them up, I am tightening a bunch of stuffs now that I have tests under control. Since the PR is already 24 files I also intend to break this down. Have changed the abstract class(which I thought I would abstract out few methods) but since I couldn't changing it back to an interface. Feel free to share your high level comments though till I get the PR in a reviewable state

Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand this at a high level, you're pulling the translog-related operations out of Engine and putting them into a TranslogManager (which is an interface with multiple implementations), and the engine holds a reference to a TranslogManager instance. The three implementations of TranslogManager here are:

  • InternalTranslogManager: this is responsible for the durability of uncommitted writes, as well as replication (i.e. document replication). This is the status quo before seg rep existed.
  • WriteOnlyTranslogManager: this is responsible for the durability of uncommitted writes, but not replication. Used when segment replication is enabled.
  • NoOpTranslogManager: does nothing. Used by NoOpEngine.

Is all this right? This seems reasonable to me. It seems like a good structural refactoring that allows for cleaner injection of different translog-related logic. It seems to line up with @nknize's suggestion from another PR as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You got that right @andrross. Thanks

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 9acad5b173fd3bc0dfb90fc4ca0cb8b735651a7a
Log 5731

Reports 5731

Signed-off-by: Bukhtawar Khan <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 62f7377d050a53972f0cfdc0f19e1275a334e897
Log 5733

Reports 5733

@Bukhtawar
Copy link
Collaborator Author

start gradle check

Signed-off-by: Bukhtawar Khan <[email protected]>
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success c5bc432
Log 5744

Reports 5744

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success c5bc432
Log 5745

Reports 5745

dependabot bot and others added 24 commits June 21, 2022 11:06
Bumps com.diffplug.spotless from 6.6.1 to 6.7.0.

---
updated-dependencies:
- dependency-name: com.diffplug.spotless
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…h-project#3357)

* Bump guava from 18.0 to 23.0 in /plugins/ingest-attachment

Bumps [guava](https://github.com/google/guava) from 18.0 to 23.0.
- [Release notes](https://github.com/google/guava/releases)
- [Commits](google/guava@v18.0...v23.0)

---
updated-dependencies:
- dependency-name: com.google.guava:guava
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>

* Updating SHAs

Signed-off-by: dependabot[bot] <[email protected]>

* Add more ingorance of using internal java API sun.misc.Unsafe

Signed-off-by: Tianli Feng <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tianli Feng <[email protected]>
Signed-off-by: Kunal Kotwani <[email protected]>

Co-authored-by: opensearch-ci-bot <[email protected]>
Upgrades to latest snapshot of lucene 9.3; including reducing maxFullFlushMergeWaitMillis 
in LuceneTest.testWrapLiveDocsNotExposeAbortedDocuments to 0 ms to ensure aborted 
docs are not merged away in the test with the new mergeOnRefresh default policy.

Signed-off-by: Nicholas Walter Knize <[email protected]>
…ch-project#3460)

* Add RemoteDirectory interface to copy segment files to/from remote store

Signed-off-by: Sachin Kale <[email protected]>

Co-authored-by: Sachin Kale <[email protected]>

* Add index level setting for remote store

Signed-off-by: Sachin Kale <[email protected]>

Co-authored-by: Sachin Kale <[email protected]>

* Add RemoteDirectoryFactory and use RemoteDirectory instance in RefreshListener

Co-authored-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>

* Upload segment to remote store post refresh

Signed-off-by: Sachin Kale <[email protected]>

Co-authored-by: Sachin Kale <[email protected]>
…opensearch-project#3571)

* Bump jettison from 1.4.1 to 1.5.0 in /plugins/discovery-azure-classic

Bumps [jettison](https://github.com/jettison-json/jettison) from 1.4.1 to 1.5.0.
- [Release notes](https://github.com/jettison-json/jettison/releases)
- [Commits](jettison-json/jettison@jettison-1.4.1...jettison-1.5.0)

---
updated-dependencies:
- dependency-name: org.codehaus.jettison:jettison
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Updating SHAs

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
…v20220608-1.32.1 in /plugins/repository-gcs (opensearch-project#3573)

* Bump google-api-services-storage in /plugins/repository-gcs

Bumps google-api-services-storage from v1-rev20200814-1.30.10 to v1-rev20220608-1.32.1.

---
updated-dependencies:
- dependency-name: com.google.apis:google-api-services-storage
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

* Updating SHAs

Signed-off-by: dependabot[bot] <[email protected]>

* Upgrade Google HTTP Client to 1.42.0

Signed-off-by: Xue Zhou <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
Co-authored-by: Xue Zhou <[email protected]>
* Add flat_skew setting to node overload decider

Signed-off-by: Rishab Nahata <[email protected]>
…search-project#3572)

* Bump xmlbeans from 5.0.3 to 5.1.0 in /plugins/ingest-attachment

Bumps xmlbeans from 5.0.3 to 5.1.0.

---
updated-dependencies:
- dependency-name: org.apache.xmlbeans:xmlbeans
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Updating SHAs

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
…gce (opensearch-project#3570)

* Bump google-oauth-client from 1.34.0 to 1.34.1 in /plugins/discovery-gce

Bumps [google-oauth-client](https://github.com/googleapis/google-oauth-java-client) from 1.34.0 to 1.34.1.
- [Release notes](https://github.com/googleapis/google-oauth-java-client/releases)
- [Changelog](https://github.com/googleapis/google-oauth-java-client/blob/main/CHANGELOG.md)
- [Commits](googleapis/google-oauth-java-client@v1.34.0...v1.34.1)

---
updated-dependencies:
- dependency-name: com.google.oauth-client:google-oauth-client
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* Updating SHAs

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>
…AllocationDecider (opensearch-project#3428)

* Fix for bug showing incorrect awareness attributes count in AwarenessAllocationDecider

Signed-off-by: Anshu Agarwal <[email protected]>
Signed-off-by: GitHub <[email protected]>

Co-authored-by: opensearch-ci-bot <[email protected]>
* Support unknown node role

Currently OpenSearch only supports several built-in nodes like data node
role. If specify unknown node role, OpenSearch node will fail to start.
This limit how to extend OpenSearch to support some extension function.
For example, user may prefer to run ML tasks on some dedicated node
which doesn't serve as any built-in node roles. So the ML tasks won't
impact OpenSearch core function. This PR removed the limitation and user
can specify any node role and OpenSearch will start node correctly with
that unknown role. This opens the door for plugin developer to run
specific tasks on dedicated nodes.

Issue: opensearch-project#2877

Signed-off-by: Yaliang Wu <[email protected]>

* fix cat nodes rest API spec

Signed-off-by: Yaliang Wu <[email protected]>

* fix mixed cluster IT failure

Signed-off-by: Yaliang Wu <[email protected]>

* add DynamicRole

Signed-off-by: Yaliang Wu <[email protected]>

* change generator method name

Signed-off-by: Yaliang Wu <[email protected]>

* fix failed docker test

Signed-off-by: Yaliang Wu <[email protected]>

* transform role name to lower case to avoid confusion

Signed-off-by: Yaliang Wu <[email protected]>

* transform the node role abbreviation to lower case

Signed-off-by: Yaliang Wu <[email protected]>

* fix checkstyle

Signed-off-by: Yaliang Wu <[email protected]>

* add test for case-insensitive role name change

Signed-off-by: Yaliang Wu <[email protected]>
…stermanager' (opensearch-project#3556)

* Rename package org.opensearch.action.support.master to org.opensearch.action.support.clustermanager

Signed-off-by: Tianli Feng <[email protected]>

* Rename classes with master term in the package org.opensearch.action.support.master

Signed-off-by: Tianli Feng <[email protected]>

* Deprecate classes in org.opensearch.action.support.master

Signed-off-by: Tianli Feng <[email protected]>

* Remove pakcage o.o.action.support.master

Signed-off-by: Tianli Feng <[email protected]>

* Move package-info back

Signed-off-by: Tianli Feng <[email protected]>

* Move package-info to new folder

Signed-off-by: Tianli Feng <[email protected]>

* Correct the package-info

Signed-off-by: Tianli Feng <[email protected]>
…h-project#3454)

* Fix false positive query timeouts due to using cached time

Signed-off-by: Ahmad AbuKhalil <[email protected]>

* delegate nanoTime call to SearchContext

Signed-off-by: Ahmad AbuKhalil <[email protected]>

* add override to SearchContext getRelativeTimeInMillis to force non cached time

Signed-off-by: Ahmad AbuKhalil <[email protected]>
Signed-off-by: Bukhtawar Khan <[email protected]>
@Bukhtawar
Copy link
Collaborator Author

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 9bf2469
Log 6172

Reports 6172

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 9bf2469
Log 6173

Reports 6173

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.