Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

remove deprecated "safety feature" in CassandraIdx #877

Closed
Dieterbe opened this issue Mar 19, 2018 · 4 comments
Closed

remove deprecated "safety feature" in CassandraIdx #877

Dieterbe opened this issue Mar 19, 2018 · 4 comments

Comments

@Dieterbe
Copy link
Contributor

in #574 we introduced these lines:

       // This is just a safety precaution to prevent corrupt index entries.
       // This ensures that the index entry always contains the correct metricDefinition data.
       if inMemory {
               archive.MetricDefinition = *schema.MetricDefinitionFromMetricData(data)
               archive.MetricDefinition.Partition = partition
       }
  1. as per update idx handling #574 (comment)
    this was only temporary, and can be removed (note that the problem we saw originally was on the Nodes attribute of the MetricDefinition which we no longer use)

  2. it doesn't work as you would expect because memoryIdx.AddOrUpdate()
    returns a copy of the Archive (both in the case of new->added, and update scenario)
    so any changes won't be seen by the memory index
    see https://play.golang.org/p/mF5hFkVD8_C
    the exception being the tags slice, if you don't update the slice you have access to the underlying array and can change it:
    https://play.golang.org/p/y4EqMLAaFxg

so the only protection that code gives is if somehow the tags within the backing array get altered.
(not when the tags slice gets overridden by a slice with different array, or a capacity-extended slice)
we have no reason to believe we have such a bug, making this code removeable

on the other hand, if we have precisely such a tag-array bug, this would expose it. we should do a sanity check of the index code before removing this.

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Mar 19, 2018

heh i totally missed the c.MemoryIdx.Update(archive) call after this code so point 2 is wrong.

that said, probably still makes sense to remove that code now, as we shouldn't need it.

@woodsaj
Copy link
Member

woodsaj commented Mar 19, 2018

lets get rid of this. We definitely dont need it anymore.

@Dieterbe
Copy link
Contributor Author

i'll do this as part of #876

@Dieterbe
Copy link
Contributor Author

merged

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants