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

bgutil: add ways to shard repairs #119

Merged
merged 1 commit into from Sep 15, 2016
Merged

bgutil: add ways to shard repairs #119

merged 1 commit into from Sep 15, 2016

Conversation

iksaif
Copy link
Contributor

@iksaif iksaif commented Sep 9, 2016

See #60.


This change is Reviewable

@iksaif
Copy link
Contributor Author

iksaif commented Sep 13, 2016

@natbraun


Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@natbraun
Copy link
Contributor

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


biggraphite/accessor.py, line 726 at r1 (raw file):

        Args:
          start_key: string, start at key >= start_key.
          end_key: string, stop at key == end_key.

Question: shouldn't this be the standard left-inclusive, right-exclusive semantics?


biggraphite/metadata_cache.py, line 236 at r1 (raw file):

        Args:
          start_key: string, start at key >= start_key.
          end_key: string, stop at key == end_key.

same as above


biggraphite/metadata_cache.py, line 249 at r1 (raw file):

                print (key, value)
                i += 1
                if end_key is not None and key > end_key:

same as above


biggraphite/drivers/cassandra.py, line 651 at r1 (raw file):

            result = self.__session.execute(statement, args)

            if not len(result.current_rows):

len == 0 seems better, I think


Comments from Reviewable

@iksaif
Copy link
Contributor Author

iksaif commented Sep 15, 2016

Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


biggraphite/accessor.py, line 726 at r1 (raw file):

Previously, natbraun (Nathaniel Braun) wrote…

Question: shouldn't this be the standard left-inclusive, right-exclusive semantics?

done

biggraphite/metadata_cache.py, line 236 at r1 (raw file):

Previously, natbraun (Nathaniel Braun) wrote…

same as above

Done.

biggraphite/metadata_cache.py, line 249 at r1 (raw file):

Previously, natbraun (Nathaniel Braun) wrote…

same as above

Done.

biggraphite/drivers/cassandra.py, line 651 at r1 (raw file):

Previously, natbraun (Nathaniel Braun) wrote…

len == 0 seems better, I think

Done.

Comments from Reviewable

@natbraun
Copy link
Contributor

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


biggraphite/metadata_cache.py, line 247 at r2 (raw file):

                    return
            for key, value in cursor:
                print (key, value)

Do we need this print here? If yes, I suggest making it more explicit, like print('Key => value: %s => %s" % (key, value))


Comments from Reviewable

@iksaif
Copy link
Contributor Author

iksaif commented Sep 15, 2016

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.


biggraphite/metadata_cache.py, line 247 at r2 (raw file):

Previously, natbraun (Nathaniel Braun) wrote…

Do we need this print here? If yes, I suggest making it more explicit, like print('Key => value: %s => %s" % (key, value))

Done.

Comments from Reviewable

@natbraun
Copy link
Contributor

Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.


biggraphite/accessor.py, line 714 at r3 (raw file):


biggraphite/accessor.py, line 721 at r3 (raw file):

        During the repair the keyspace is split in nshards and
        this function will only take car of 1/n th of the data
        as specified by shard. This allows the caller to parallelize

take care


Comments from Reviewable

@natbraun
Copy link
Contributor

:lgtm_strong: after my comment is fixed


Review status: 0 of 6 files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

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