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

[Remove] Segment memory estimation and tracking #2029

Merged
merged 4 commits into from
Feb 4, 2022

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Feb 1, 2022

Lucene 9 removed CodecReader#ramBytesUsed and all file formats that no longer
consume large amounts of memory. As a result RAM estimation for segments is no
longer possible and is removed in order to upgrade to lucene 9. backwards compatibility is retained for the transport layer.

@nknize nknize added v2.0.0 Version 2.0.0 deprecate labels Feb 1, 2022
@nknize nknize requested a review from a team as a code owner February 1, 2022 18:40
@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure a649a14fa58df6113d02ce12f8a259b3ef625ff9
Log 2151

Reports 2151

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure ee32118c2ddcf37eaeb5bc66feecb8555a6d617a
Log 2155

Reports 2155

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure b3f066040127120242b920df90ed956ef022387d
Log 2158

Reports 2158

Lucene 9 removed CodecReader#ramBytesUsed and all file formats that no longer
consume large amounts of memory. As a result RAM estimation for segments is no
longer possible and needs to be removed. backwards compatibility is retained for
the tranport layer.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize nknize force-pushed the remove/segmentMemoryEstimate branch from b3f0660 to 693ed1c Compare February 1, 2022 22:04
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 693ed1c
Log 2161

Reports 2161

@dblock dblock requested a review from andrross February 2, 2022 19:07
Copy link
Contributor

@adnapibar adnapibar left a comment

Choose a reason for hiding this comment

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

👍

table.addCell(totalStats.getSegments() == null ? null : totalStats.getSegments().getMemory());
table.addCell(primaryStats.getSegments() == null ? null : primaryStats.getSegments().getMemory());
table.addCell(totalStats.getSegments() == null ? null : totalStats.getSegments().getZeroMemory());
table.addCell(primaryStats.getSegments() == null ? null : primaryStats.getSegments().getZeroMemory());

Copy link
Contributor

Choose a reason for hiding this comment

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

why are we not removing these instead - for backward compatibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, they're needed for bwc so we just report 0 memory usage.


@Override
public void accept(OpenSearchDirectoryReader reader, OpenSearchDirectoryReader previousReader) {
final CircuitBreaker breaker = breakerService.getBreaker(CircuitBreaker.ACCOUNTING);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we removed it, I believe CircuitBreaker.ACCOUNTING becomes useless as well, seems like this is the only place where it was managed (please correct me if I am wrong).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good catch. We can remove the Accountable. I'll take care of that here 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.

On second thought. I'm going to remove that in a follow up PR to keep the changes contained.

@nknize nknize merged commit fc0d3a3 into opensearch-project:main Feb 4, 2022
Yury-Fridlyand added a commit to Bit-Quill/opensearch-net that referenced this pull request Jun 9, 2022
`OpenSearch` 2.0 uses newer version of `Lucene` (9.0) which doesn't provide segments stats info.

Ref: opensearch-project/OpenSearch#2029 opensearch-project/OpenSearch#1109
See also history for `server/src/main/java/org/opensearch/index/engine/SegmentsStats.java` in `OpenSearch` repo.

Signed-off-by: Yury Fridlyand <yuryf@bitquilltech.com>
Yury-Fridlyand added a commit to Bit-Quill/opensearch-net that referenced this pull request Aug 29, 2022
* Update nuget packages (opensearch-project#88)
* Cleanup - removing stale dependencies.
* Fix 2 typos in scripts project
* Update `scripts` project file to include all relevant objects.
* Rename `master` node role to `cluster_manager` as it was done in OpenSearch.
    Ref: opensearch-project/OpenSearch#2480
* Remove validation for indices segments stats.
    `OpenSearch` 2.0 uses newer version of `Lucene` (9.0) which doesn't provide segments stats info.
    Ref: opensearch-project/OpenSearch#2029 opensearch-project/OpenSearch#1109
    See also history for `server/src/main/java/org/opensearch/index/engine/SegmentsStats.java` in `OpenSearch` repo.
* Remove tests for `_type` validation in mapping APIs as it was removed from `OpenSearch`.
    Ref: opensearch-project/OpenSearch#2238 opensearch-project/OpenSearch#2480
* Remove usage of deprecated `search.remote` settings.
    Ref: opensearch-project/OpenSearch#1870
* Update abstractions package - patch to support OpenSearch 2.0. Update integration workflow to run tests on OpenSearch 2.0.
* Rename `master_timeout` to `cluster_manager_timeout` in all APIs where it is used.
* Enrich comments to already renamed `CatMaster`/`CatClusterManager` API.
* Rename in `/_cluster/stats`/`cluster.stats` and `/_cluster/state`/`cluster.state`.
* Add deprecation info.
* Rename in comments.
* Rename in test data.
* Renamings in tests including `MasterEligible`, but mark it is obsolete.
* Rename branch reference in scripting.
* Mark `indices.exists_type`/`TypeExists` APIs as deprecated.
* Update compatibility matrix and include it into `sln` file.
* Add deprecation notice to all reference of `include_type_name`/`IncludeTypeName`.
* Update compatibility matrix.
* Remove `OpenDistro` compatibility notice.
* Update repo link.
* Add small README for each project being released.
* Address PR opensearch-project#51 feedback.

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
wbeckler pushed a commit to opensearch-project/opensearch-net that referenced this pull request Aug 31, 2022
* Update nuget packages (#88)
* Cleanup - removing stale dependencies.
* Fix 2 typos in scripts project
* Update `scripts` project file to include all relevant objects.
* Rename `master` node role to `cluster_manager` as it was done in OpenSearch.
    Ref: opensearch-project/OpenSearch#2480
* Remove validation for indices segments stats.
    `OpenSearch` 2.0 uses newer version of `Lucene` (9.0) which doesn't provide segments stats info.
    Ref: opensearch-project/OpenSearch#2029 opensearch-project/OpenSearch#1109
    See also history for `server/src/main/java/org/opensearch/index/engine/SegmentsStats.java` in `OpenSearch` repo.
* Remove tests for `_type` validation in mapping APIs as it was removed from `OpenSearch`.
    Ref: opensearch-project/OpenSearch#2238 opensearch-project/OpenSearch#2480
* Remove usage of deprecated `search.remote` settings.
    Ref: opensearch-project/OpenSearch#1870
* Update abstractions package - patch to support OpenSearch 2.0. Update integration workflow to run tests on OpenSearch 2.0.
* Rename `master_timeout` to `cluster_manager_timeout` in all APIs where it is used.
* Enrich comments to already renamed `CatMaster`/`CatClusterManager` API.
* Rename in `/_cluster/stats`/`cluster.stats` and `/_cluster/state`/`cluster.state`.
* Add deprecation info.
* Rename in comments.
* Rename in test data.
* Renamings in tests including `MasterEligible`, but mark it is obsolete.
* Rename branch reference in scripting.
* Mark `indices.exists_type`/`TypeExists` APIs as deprecated.
* Update compatibility matrix and include it into `sln` file.
* Add deprecation notice to all reference of `include_type_name`/`IncludeTypeName`.
* Update compatibility matrix.
* Remove `OpenDistro` compatibility notice.
* Update repo link.
* Add small README for each project being released.
* Address PR #51 feedback.

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>

Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecate v2.0.0 Version 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants