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

fix: add missing translog sync interval option to index settings #518

Conversation

Mstrutov
Copy link
Contributor

@Mstrutov Mstrutov commented Jun 6, 2023

Description

Now we can set translog sync interval option via client.

Additionally, server response contains separate translog object, which cannot be deserialized by present translog option deserializers.
It looks like this:

{
  "test-settings": {
    "settings": {
      "index": {
        "number_of_shards": "1",
        "translog": {
          "sync_interval": "10s",
          "durability": "async"
        },
        "provided_name": "test-settings",
        "creation_date": "1685967681952",
        "number_of_replicas": "1",
        "uuid": "q4R3eDKjT164eR_NNuY8pQ",
        "version": {
          "created": "136257827"
        }
      }
    }
  }
}

Therefore, a corresponding Translog class is introduced, similarly to how it is done for the blocks property.

Issues Resolved

Closes #307

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.

@Mstrutov Mstrutov force-pushed the fix/add-translog-sync-interval-to-index-settings branch 2 times, most recently from 5ecc6c7 to 6ba68d8 Compare June 6, 2023 04:59
reta
reta previously approved these changes Jun 7, 2023
@Mstrutov Mstrutov force-pushed the fix/add-translog-sync-interval-to-index-settings branch from f849a4e to ff2f996 Compare June 8, 2023 12:10
@Mstrutov Mstrutov requested a review from reta June 8, 2023 12:11
@Mstrutov Mstrutov force-pushed the fix/add-translog-sync-interval-to-index-settings branch from ff2f996 to 410e08f Compare June 8, 2023 12:28
reta
reta previously approved these changes Jun 8, 2023
@Mstrutov Mstrutov dismissed stale reviews from dblock and reta via 8d06598 June 13, 2023 05:28
@Mstrutov Mstrutov force-pushed the fix/add-translog-sync-interval-to-index-settings branch 2 times, most recently from 8d06598 to 891e0fe Compare June 13, 2023 05:35
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I have some nice to have's but this is good by me. Up to you if you want to address them.

dblock
dblock previously approved these changes Jun 13, 2023
@Mstrutov Mstrutov force-pushed the fix/add-translog-sync-interval-to-index-settings branch 2 times, most recently from b7d50ca to 86078bb Compare June 15, 2023 07:36
@Mstrutov Mstrutov requested a review from dblock June 15, 2023 08:01
dblock
dblock previously approved these changes Jun 20, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@dblock dblock requested a review from reta June 20, 2023 12:26
@Mstrutov Mstrutov requested a review from dblock June 22, 2023 06:40
dblock
dblock previously approved these changes Jun 26, 2023
@dblock
Copy link
Member

dblock commented Jun 26, 2023

LGTM! @reta?

reta
reta previously approved these changes Jun 27, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
VachaShah
VachaShah previously approved these changes Jun 27, 2023
@Mstrutov Mstrutov dismissed stale reviews from VachaShah, reta, and dblock via a58b899 June 28, 2023 10:51
@Mstrutov Mstrutov force-pushed the fix/add-translog-sync-interval-to-index-settings branch from 4484d87 to a58b899 Compare June 28, 2023 10:51
Mstrutov added 3 commits June 28, 2023 13:53
Additionally, server response contains separate `translog` object,
which cannot be deserialized by present translog option deserializers.

Therefore, this commit introduces a corresponding `Translog` class,
similarly to how it is done for the `blocks` property.

Signed-off-by: Maksim Strutovskii <[email protected]>
Now that all the translog options are stored in a separate object, we
should remove the separate fields to avoid confusion.

Public methods (getters, builder setters) should remain intact, mark
them deprecated and point them to the corresponding translog fields.

Deserializer should still recognize string-valued translog attributes, keep
the deprecated setters as field deserializers.

Serializer does not have to produce string-valued translog attributes,
as server parses `translog` attribute. Do not write translog contents to
old `translog.xxx` fields.

Signed-off-by: Maksim Strutovskii <[email protected]>
@Mstrutov Mstrutov force-pushed the fix/add-translog-sync-interval-to-index-settings branch from a58b899 to e0f63b5 Compare June 28, 2023 10:53
@Mstrutov Mstrutov requested a review from reta June 28, 2023 10:54
@Mstrutov Mstrutov requested a review from dblock June 28, 2023 12:43
Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

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

Thank you @Mstrutov!

@reta reta merged commit 1323796 into opensearch-project:main Jun 28, 2023
@reta reta added the backport 2.x Backport to 2.x branch label Jun 28, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 28, 2023
* fix: add missing translog sync interval option to index settings

Additionally, server response contains separate `translog` object,
which cannot be deserialized by present translog option deserializers.

Therefore, this commit introduces a corresponding `Translog` class,
similarly to how it is done for the `blocks` property.

Signed-off-by: Maksim Strutovskii <[email protected]>

* fix: encapsulate translog options into translog in index settings

Now that all the translog options are stored in a separate object, we
should remove the separate fields to avoid confusion.

Public methods (getters, builder setters) should remain intact, mark
them deprecated and point them to the corresponding translog fields.

Deserializer should still recognize string-valued translog attributes, keep
the deprecated setters as field deserializers.

Serializer does not have to produce string-valued translog attributes,
as server parses `translog` attribute. Do not write translog contents to
old `translog.xxx` fields.

Signed-off-by: Maksim Strutovskii <[email protected]>

* refactor: use ModelTestCase in tests for json serialization/deserialization

Signed-off-by: Maksim Strutovskii <[email protected]>

---------

Signed-off-by: Maksim Strutovskii <[email protected]>
(cherry picked from commit 1323796)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request Jun 28, 2023
… (#544)

* fix: add missing translog sync interval option to index settings

Additionally, server response contains separate `translog` object,
which cannot be deserialized by present translog option deserializers.

Therefore, this commit introduces a corresponding `Translog` class,
similarly to how it is done for the `blocks` property.



* fix: encapsulate translog options into translog in index settings

Now that all the translog options are stored in a separate object, we
should remove the separate fields to avoid confusion.

Public methods (getters, builder setters) should remain intact, mark
them deprecated and point them to the corresponding translog fields.

Deserializer should still recognize string-valued translog attributes, keep
the deprecated setters as field deserializers.

Serializer does not have to produce string-valued translog attributes,
as server parses `translog` attribute. Do not write translog contents to
old `translog.xxx` fields.



* refactor: use ModelTestCase in tests for json serialization/deserialization



---------


(cherry picked from commit 1323796)

Signed-off-by: Maksim Strutovskii <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@BrendonFaleiro BrendonFaleiro mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Missing Index Setting Options
4 participants