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 initial update alias op to ES7 #2920

Merged
merged 4 commits into from Jul 3, 2020
Merged

Conversation

6c697a61
Copy link
Contributor

@6c697a61 6c697a61 commented Apr 27, 2020

Hi @clairemcginty and @regadas, can we add a few methods here?

Related to #3132

* @param newIndexName index to point the alias to
* @param timeout defaults to 1 minute
*/
def swapIndexAlias(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can make this a little bit more broader.

What about renaming this to createAlias and have a removePrevious: Boolean and a isWriteIndex: Boolean. I think it would allow more use cases.

Copy link
Contributor Author

@6c697a61 6c697a61 May 15, 2020

Choose a reason for hiding this comment

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

Oops, missed this comment, sorry @regadas. To make sure I understood you right:
createAlias would just add a new index alias
removePrevious would unlink a given alias from all indices

What does isWriteIndex do?
I assume the Boolean would be derived from Try(AcknowledgedResponse) by applying x.map(_.isAchnowledged).getOrElse(false)

Would be kinda great to still add a swap method since that's what we do when we create index from scratch every time

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @6c697a61, ES alias can point to multiple indices and isWriteIndex will tell that a given index is the one to write to when using the alias to write. (It wouldn't actually be a Boolean in this case).

I think swapping can be achieved by using smth like createAlias(alias, index, removePrevious = true) without having specific swap op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, have them as additional parameters! Got confused, sorry - thought you meant functions.
Adding removePrevious , but still not clear of how isWriteIndex is intended to be used. I understand what it is, just want to make sure we are on the same page about how it should be used. Do you mean we could put it as an additional parameter in ensureIndex to be able to prevent writing to an aliasedIndex @regadas?

@regadas regadas changed the title Can we expand it a bit? Add extra Elasticsearch admin ops Apr 28, 2020
@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #2920 into master will decrease coverage by 2.63%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2920      +/-   ##
==========================================
- Coverage   72.66%   70.03%   -2.64%     
==========================================
  Files         224      224              
  Lines        7452     7452              
  Branches      312      330      +18     
==========================================
- Hits         5415     5219     -196     
- Misses       2037     2233     +196     
Impacted Files Coverage Δ
...scala/com/spotify/scio/bigquery/client/Cache.scala 0.00% <0.00%> (-74.08%) ⬇️
.../spotify/scio/bigquery/client/BigQueryConfig.scala 0.00% <0.00%> (-70.59%) ⬇️
.../spotify/scio/bigquery/BigQueryPartitionUtil.scala 0.00% <0.00%> (-52.50%) ⬇️
...scala/com/spotify/scio/bigquery/BigQueryUtil.scala 50.00% <0.00%> (-50.00%) ⬇️
...otify/scio/bigquery/syntax/ScioContextSyntax.scala 19.35% <0.00%> (-41.94%) ⬇️
...la/com/spotify/scio/bigquery/client/QueryOps.scala 0.73% <0.00%> (-39.71%) ⬇️
.../scala/com/spotify/scio/bigquery/StorageUtil.scala 0.00% <0.00%> (-36.85%) ⬇️
...com/spotify/scio/bigquery/types/BigQueryType.scala 58.33% <0.00%> (-29.17%) ⬇️
...la/com/spotify/scio/bigquery/client/TableOps.scala 0.00% <0.00%> (-24.25%) ⬇️
...com/spotify/scio/bigquery/types/TypeProvider.scala 52.11% <0.00%> (-22.54%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 800891b...91250e2. Read the comment docs.

Copy link
Contributor

@regadas regadas left a comment

Choose a reason for hiding this comment

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

Hi @6c697a61 sorry for the late reply. Left a few comments and do you think we can extend this to the other ES versions?

timeout: TimeValue
): AcknowledgedResponse = {

val getAliasesResponse =
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be moves after L#208

Comment on lines +201 to +206
val request = new IndicesAliasesRequest()
.addAliasAction(
new AliasActions(AliasActions.Type.ADD)
.index(indexName)
.alias(alias)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant with isWriteIndex: Boolean was smth along the lines of:

Suggested change
val request = new IndicesAliasesRequest()
.addAliasAction(
new AliasActions(AliasActions.Type.ADD)
.index(indexName)
.alias(alias)
)
// indices: Iterable[(String, Boolean)] (index name, is write index)
require(
indices.find(_._2).size == 1,
"Only one index per alias can be assigned to be the write index at a time"
val request = indices.foldLeft(new IndicesAliasesRequest()) {
case (request, (idx, isWriteIndex)) =>
request.addAliasAction(
new AliasActions(AliasActions.Type.ADD)
.index(idx)
.writeIndex(isWriteIndex)
.alias(alias)
)
}

* @param timeout defaults to 1 minute
*/
def createOrUpdateAlias(
nodes: Iterable[HttpHost],
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if we just have methods with ElasticsearchOptions

@regadas regadas self-requested a review May 30, 2020 13:34
Copy link
Contributor

@regadas regadas left a comment

Choose a reason for hiding this comment

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

I think ES 5 will be the only version that doesn't have the same ES alias behaviour.

@nevillelyh
Copy link
Contributor

This has been ongoing for a while? What needs to be done to get it merged?

@regadas regadas changed the title Add extra Elasticsearch admin ops Add initial update alias op to ES7 Jul 3, 2020
@regadas regadas merged commit 4e98c46 into spotify:master Jul 3, 2020
regadas pushed a commit that referenced this pull request Jul 3, 2020
Co-authored-by: Liza Meleshchuk <elizavetam@spotify.com>
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.

None yet

3 participants