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

IndexSet default setting #3209

Merged
merged 7 commits into from Dec 16, 2016
Merged

IndexSet default setting #3209

merged 7 commits into from Dec 16, 2016

Conversation

bernd
Copy link
Member

@bernd bernd commented Dec 15, 2016

An index set can be marked as default in IndexSetConfig. This cannot be changed by the user at the moment, but should be. We have to ensure that only one index set is the default!

Since we are using MongoDB, there are no cross-document atomic operations or transactions so we have to take care of that ourselves.

Idea 1: Use a timestamp

We are currently using a boolean default field in the index_sets database collection to mark the default index set. Because of the lack of transactions this is not really safe to update for several documents at once. (set true on the new default, set false on all others)

As an alternative to the boolean flag we could use a timestamp field like default_since or similar that will be set to the current time when a "set default index set" request comes in. To determine the default we have to load the index set with the newest timestamp. By using a unique index on the timestamp field, we would avoid duplicate timestamps.

Idea 2: Coordinate on master

Another idea was to publish an event on the cluster event bus to set the default index set. All nodes subscribe to the event but only the master node will actually handle it. Since only one node should be master and the events are not processed in parallel, this should avoid interleaving writes to the index sets collection. It doesn't guarantee any order though.

This is more complicated than the timestamp idea beacuse it involves the cluster event bus and needs an event handler. It also makes the request to set the default index set async so we have to deal with that in the UI. It also fails when there is more than one active master node.

Other Ideas?

Please let me know if you have different, better ideas. Let's discuss!

@bernd bernd added this to the 2.2.0 milestone Dec 15, 2016
@kroepke
Copy link
Member

kroepke commented Dec 15, 2016

Can we simply use a ClusterConfig (or similar) setting that contains the id (or name or both) of the current default? That way there's always exactly 0 or 1 default index sets.

@joschi
Copy link
Contributor

joschi commented Dec 15, 2016

@bernd Idea 3:

Store the default index set ID in cluster config. This would make queries for that more expensive, but in how many places do we really have to check if an index set is the default index set?

@bernd
Copy link
Member Author

bernd commented Dec 15, 2016

@joschi @kroepke Good point! That seems to be the simplest idea, I guess. Thank you!

@joschi Right now we only need it for display purposes in the UI and when importing content packs.

@bernd
Copy link
Member Author

bernd commented Dec 15, 2016

This is ready for review now.

@@ -38,6 +38,13 @@
Optional<IndexSetConfig> get(String id);

/**
* Retrieve the default index set.
*
* @return A filled {@link Optional} with the default index set, an empty {@link Optional} if there is no default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a situation where there is no default index set, which is not a serious bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default index set should be there. If it's not then

  • Content pack imports that contain streams will fail
  • The old indices REST API endpoints without an index set parameter will fail
  • The V20161116172200_CreateDefaultStreamMigration will fail

If we change that to not returning an optional and throwing an error when the default index set is absent, we also need a migration to migrate the default flag from the index_sets collection to the cluster config object.

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 that would be preferable to handling an Optional<T> in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, damn. Forgot to change the return type of IndexSetService#getDefault().

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted the interfaces.

@joschi joschi self-assigned this Dec 15, 2016
@bernd
Copy link
Member Author

bernd commented Dec 16, 2016

I added a migration to create a DefaultIndexSetConfig cluster config object and delete the default field from the index set documents.

…setting

This removes the `default` field from the IndexSetConfig database object
and creates a DefaultIndexSetConfig object in the cluster config
instead. Using a cluster config object makes it easier to ensure that
only one index set is the default.

There is no migration to convert the `default` flag to a
DefaultIndexSetConfig in the cluster config because this only happened
to people using alphas and betas and the user is able to just set the
default index set via the API or in the UI. (UI upcoming)

Also implement the IndexSetsResource#setDefault() method to actually set
the default in cluster config.
@bernd
Copy link
Member Author

bernd commented Dec 16, 2016

Rebased against master.

@@ -529,6 +529,12 @@
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.github.fakemongo</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a duplicate:

<dependency>
<groupId>com.github.fakemongo</groupId>
<artifactId>fongo</artifactId>
<version>${fongo.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jukito</groupId>
<artifactId>jukito</artifactId>
<version>${jukito.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.github.fakemongo</groupId>
<artifactId>fongo</artifactId>
<version>${fongo.version}</version>
<scope>test</scope>
</dependency>

Copy link
Member Author

Choose a reason for hiding this comment

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

Lol, for some reason I couldn't import the FongoRule class before adding this. Must have been something else. :trollface:

Since the default index must be there, we do not return an Optional but
throw an IllegalStateException if the default index set cannot be found.
indexSet.cycle();
indexSet.cycle();
} catch (IllegalStateException e) {
throw new NotFoundException("Default index set not found");
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 throw an InternalServerErrorException here (or simply let the IllegalStateException bubble up which results in the same) because it's actually an error in the Graylog system, not a wrong request by the client.

Copy link
Member Author

Choose a reason for hiding this comment

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

final IndexSet indexSet = indexSetRegistry.getDefault();
return DeflectorSummary.create(indexSet.isUp(), indexSet.getCurrentActualTargetIndex());
} catch (IllegalStateException e) {
throw new NotFoundException("Default index set not found");
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 throw an InternalServerErrorException here (or simply let the IllegalStateException bubble up which results in the same) because it's actually an error in the Graylog system, not a wrong request by the client.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joschi joschi merged commit 2ac7e88 into master Dec 16, 2016
@bernd bernd deleted the default-index-set-option branch December 16, 2016 16:56
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