Refactor index ranges handling #1274
Merged
Conversation
added 17 commits
Jun 25, 2015
added 8 commits
Jun 30, 2015
IndexResponse#isCreated() is true when the document was created and false if the document was updated.
|
|
||
| @Override | ||
| public SortedSet<IndexRange> findAll() { | ||
| final SearchRequest request = client.prepareSearch() |
kroepke
Jul 1, 2015
Member
I think this search also needs to take into account the graylog index prefix.
Otherwise we could pull in indices that could be part of another graylog deployment writing to the same cluster. (Unlikely but that would trigger very fun bugs 🎅 )
I think this search also needs to take into account the graylog index prefix.
Otherwise we could pull in indices that could be part of another graylog deployment writing to the same cluster. (Unlikely but that would trigger very fun bugs
joschi
Jul 1, 2015
Author
Contributor
Good catch. This should actually be added to all methods making queries to Elasticsearch.
Good catch. This should actually be added to all methods making queries to Elasticsearch.
| @Singleton | ||
| public class IndexMapping { | ||
| public static final String TYPE_MESSAGE = "message"; | ||
| public static final String TYPE_META = "graylog_meta"; |
kroepke
Jul 1, 2015
Member
should this maybe be called something more specific?
if we ever decide to store other metadata about the index the name would maybe be a little misleading?
also, since we control the types in the index, i think we can drop the graylog_ prefix.
perhaps indexrange ?
should this maybe be called something more specific?
if we ever decide to store other metadata about the index the name would maybe be a little misleading?
also, since we control the types in the index, i think we can drop the graylog_ prefix.
perhaps indexrange ?
joschi
Jul 1, 2015
Author
Contributor
Good point. I'll rename it.
Good point. I'll rename it.
| } | ||
|
|
||
| @Override | ||
| public void destroyAll() { |
kroepke
Jul 1, 2015
Member
Is this method used outside of tests?
Is this method used outside of tests?
joschi
Jul 1, 2015
Author
Contributor
No, it's a remainder of the incremental refactoring.
No, it's a remainder of the incremental refactoring.
| } | ||
|
|
||
| @Override | ||
| public void destroy(String index) { |
kroepke
Jul 1, 2015
Member
Is this method used outside of tests?
Is this method used outside of tests?
joschi
Jul 1, 2015
Author
Contributor
Nope, also a remainder of the incremental refactoring.
Nope, also a remainder of the incremental refactoring.
| public abstract String indexName(); | ||
|
|
||
| @JsonProperty | ||
| public abstract DateTime begin(); |
kroepke
Jul 1, 2015
Member
This changes public API, can we either keep it as start() or alias it?
This changes public API, can we either keep it as start() or alias it?
joschi
Jul 1, 2015
Author
Contributor
"start" would be the same as "end" (it was misnamed from the start – pun intended) and I think that would be much more irritating than just changing the format.
"start" would be the same as "end" (it was misnamed from the start – pun intended) and I think that would be much more irritating than just changing the format.
kroepke
Jul 7, 2015
Member
Ok, then let's somehow make this clear in the release notes?
Ok, then let's somehow make this clear in the release notes?
| @@ -108,7 +93,9 @@ public void execute() { | |||
| } | |||
|
|
|||
| try { | |||
| ranges.add(calculateRange(index)); | |||
| final IndexRange indexRange = indexRangeService.calculateRange(index); | |||
| indexRangeService.save(indexRange); | |||
kroepke
Jul 1, 2015
Member
so much 👍 for not destroying all and then adding them back!
so much
joschi
Jul 1, 2015
Author
Contributor
|
lgtm |
This was referenced Aug 5, 2015
|
Is 1.2 rc2 meant to fully address this? I'm still getting blank searches for specific time ranges until IIrecalculate. |
|
@drewmmiranda You need to recalculate index ranges once (although the old ones should've been migrated on the first start of Graylog 1.2.0, see #1378). Do you get the blank searches while running Graylog 1.2.0 or just after the upgrade from an earlier version? |
|
Running v1.2.0-rc.2. This is correct. Same behavior as before. Relative and keyword ranges work correctly. Absolute ranges do not. |
|
@drewmmiranda Please open a new ticket and elaborate on what "Relative and keyword ranges work correctly. Absolute ranges do not." means, e. g. describe test cases or what results you expect and what the actual results are. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
This PR refactors the index ranges calculation and handling in Graylog. It moves the actual persistence from MongoDB ("index_ranges" collection) into Elasticsearch ("graylog_meta" type) so that this information is stored along the Elasticsearch indices they refer to.
Additionally the index ranges now not only contain the (misnamed) "start" attribute but actually the whole index range with begin (oldest message) and end (most recent message) which can be used to optimize query requests by more precisely restricting those to the actually relevant indices.