Add cassandra storage #2

adejanovski · 2016-10-17T11:18:44Z

Adds Cassandra as a storage option for reaper internal data.
I've also added checks so that we cannot run two concurrent repair sessions for the same repair unit, and so that we cannot create two different repair schedules that have the same table in it.

michaelsembwever · 2016-10-17T19:08:39Z

pom.xml

@@ -1,156 +1,167 @@
 <?xml version="1.0" encoding="UTF-8"?>


non-blocking:
the diff of this file is hard to read with the reformatting (i presume from your IDE) changing more of the file than is explicit to this PR. it's help a lot if such reformatting changes are kept isolated in separate commits, or if you can disable reformatting in your IDE.

I'll try to do major reformattings in separate commits, you're right.

michaelsembwever · 2016-10-17T22:29:17Z

src/main/db/reaper_cassandra_db.cql

+
+CREATE TABLE IF NOT EXISTS repair_run (
+  id                 bigint PRIMARY KEY,
+  cluster_name       TEXT,


nit: sometimes text is uppercase, sometimes its lowercase…? same with int.

Indeed. I fixed formatting to be consistent

michaelsembwever · 2016-10-18T01:08:02Z

src/main/java/com/spotify/reaper/resources/CommonTools.java

+
+    Collection<RepairSchedule> repairSchedules = context.storage.getRepairSchedulesForClusterAndKeyspace(repairUnit.getClusterName(), repairUnit.getKeyspaceName());
+    for(RepairSchedule sched:repairSchedules){
+    	Optional<RepairUnit> repairUnitForSched = context.storage.getRepairUnit(sched.getRepairUnitId());


blocking: replace tabs with whitespace. (happens in many of the edited files in this PR)

Changed to spaces by default.

michaelsembwever · 2016-10-18T01:09:20Z

src/main/java/com/spotify/reaper/resources/CommonTools.java

+    		if((repairUnitForSched.get().getColumnFamilies().size()==0 && repairUnit.getColumnFamilies().size()==0)
+    				|| repairUnitForSched.get().getColumnFamilies().size()==0 && repairUnit.getColumnFamilies().size()!=0
+    				|| repairUnitForSched.get().getColumnFamilies().size()!=0 && repairUnit.getColumnFamilies().size()==0
+    				|| Sets.intersection(repairUnit.getColumnFamilies(),repairUnitForSched.get().getColumnFamilies()).size() >0){


non-blocking:
could be made easier to read: if these two if statement conditionals were made into methods with intuitive descriptive names.

eg (names could be better…)

if (isEqual(repairUnitForSched, repairUnit) { if (isRepairInProgress(repairUnitForSched, repairUnit)) { String errMsg = String.format( "A repair schedule already exists for cluster \"%s\",keyspace \"%s\", and column families: %s", cluster.getName(), repairUnit.getKeyspaceName(), Sets.intersection(repairUnit.getColumnFamilies(),repairUnitForSched.get().getColumnFamilies())); LOG.error(errMsg); throw new ReaperException(errMsg); } } private static boolean isValid(Optional<RepairUnit> repairUnitForSched, RepairUnit repairUnit) { return repairUnitForSched.isPresent() && repairUnitForSched.get().getClusterName().equals(repairUnit.getClusterName()) && repairUnitForSched.get().getKeyspaceName().equals(repairUnit.getKeyspaceName()); } private static boolean isRepairInProgress(Optional<RepairUnit> repairUnitForSched, RepairUnit repairUnit) { return (repairUnitForSched.get().getColumnFamilies().size()==0 && repairUnit.getColumnFamilies().size()==0) || repairUnitForSched.get().getColumnFamilies().size()==0 && repairUnit.getColumnFamilies().size()!=0 || repairUnitForSched.get().getColumnFamilies().size()!=0 && repairUnit.getColumnFamilies().size()==0 || Sets.intersection(repairUnit.getColumnFamilies(),repairUnitForSched.get().getColumnFamilies()).size() >0; }

Right, I refactored the condition into a separate method.
Good catch

michaelsembwever · 2016-10-18T01:09:42Z

src/main/java/com/spotify/reaper/resources/RepairRunResource.java

@@ -62,6 +63,7 @@
  private static final Logger LOG = LoggerFactory.getLogger(RepairRunResource.class);

  private final AppContext context;
+  private Boolean alreadyRunning;


where is this used?

michaelsembwever · 2016-10-18T01:09:44Z

src/main/java/com/spotify/reaper/resources/RepairRunResource.java

+    // Check that no other repair run exists with a status different than DONE for this repair unit
+    Collection<RepairRun> repairRuns = context.storage.getRepairRunsForUnit(repairRun.get().getRepairUnitId());
+
+    boolean alreadyRunning = false;


where is this used?

michaelsembwever · 2016-10-18T01:09:47Z

src/main/java/com/spotify/reaper/service/SchedulingManager.java

@@ -30,7 +30,7 @@ public static void start(AppContext context) {
      LOG.info("Starting new SchedulingManager instance");
      schedulingManager = new SchedulingManager(context);
      Timer timer = new Timer("SchedulingManagerTimer");
-      timer.schedule(schedulingManager, 1000, 1000 * 60 * 10); // activate once per ten minutes
+      timer.schedule(schedulingManager, 1000, 1000 * 60); // activate once per ten minutes


is the comment still true?

Alas, it is not 😄
Removed

michaelsembwever · 2016-10-18T01:09:57Z

src/main/java/com/spotify/reaper/storage/CassandraStorage.java

+	@Override
+	public Collection<RepairScheduleStatus> getClusterScheduleStatuses(String clusterName) {
+		// TODO Auto-generated method stub
+		return null;


I did forgot those two indeed 🤔
They are implemented now, thanks !

michaelsembwever · 2016-10-24T05:31:15Z

LGTM.

remember to collapse (git rebase -i HEAD~9) your commits before merging.
along with rewriting the collapsed commit it's really helpful to link from the commit msg back to this PR.

rustyrazorblade · 2016-10-24T05:33:17Z

Easiest to merge with --squash, it does it for you.

adejanovski · 2016-10-25T14:33:37Z

Squashed + rebased

michaelsembwever · 2016-10-25T19:40:23Z

LGTM

zznate · 2016-10-27T09:04:38Z

Oops. Looks like this butted heads with pull 3. @adejanovski can you rebase and double check the pom.xml merge?

fixed version, fixed build script Add cassandra as a storage option Prevent concurrent active repair runs on the same keyspace/table Prevent creation of multiple schedules for the same keyspace/table Fix formatting (tabs -> spaces) Refactor big fat condition into a method Removed obsolete comment Fix case on types to have a consistent set of statements Remove unused variables Implement the last two methods of IStorage : getClusterScheduleStatuses and getClusterRunStatuses Switched some logs to DEBUG level to unclutter the output Switch log level back to INFO in the config file

adejanovski · 2016-10-28T09:40:53Z

Rebased and fixed conflict.
You can merge now.

…ersions-integration-test Make the schema BasicSteps creates for integration tests compatible with Cassandra 1.2

> This is a combination of 5 commits. > This is the 1st commit message: Enable errexit > This is the commit message thelastpickle#2: Refactor JAR search logic This adds to code reuse. > This is the commit message thelastpickle#3: A bit more concise CONFIG_PATH selection Not really useful, but avoids the unnecessary string literal duplication. > This is the commit message thelastpickle#4: Refine REAPER_JAR search Do not search from '/' to avoid log cluttering and save startup time. And anyway, searching from '/' kills the very idea of searching for a *user* JAR (as apposed to a system JAR) and also makes the search result less predictable (as *all* JARs will eventually be found). The main motivation was that systemd services start with '/' as the working directory, so the system log gets filled with errors on each service start. While b970844 fixes searching from '/' for systemd service, still an explicit check pursues additional goals. > This is the commit message thelastpickle#5: Add SSL/TLS support To support clusters that have their JMX protected with SSL/TLS encryption, let us allow to configure the respective Java parameters.

> This is a combination of 5 commits. > This is the 1st commit message: Enable errexit > This is the commit message #2: Refactor JAR search logic This adds to code reuse. > This is the commit message #3: A bit more concise CONFIG_PATH selection Not really useful, but avoids the unnecessary string literal duplication. > This is the commit message #4: Refine REAPER_JAR search Do not search from '/' to avoid log cluttering and save startup time. And anyway, searching from '/' kills the very idea of searching for a *user* JAR (as apposed to a system JAR) and also makes the search result less predictable (as *all* JARs will eventually be found). The main motivation was that systemd services start with '/' as the working directory, so the system log gets filled with errors on each service start. While b970844 fixes searching from '/' for systemd service, still an explicit check pursues additional goals. > This is the commit message #5: Add SSL/TLS support To support clusters that have their JMX protected with SSL/TLS encryption, let us allow to configure the respective Java parameters.

Co-authored-by: Miles-Garnsey <miles.garnsey@datastax.com>

adejanovski assigned rustyrazorblade Oct 17, 2016

michaelsembwever reviewed Oct 17, 2016

View reviewed changes

michaelsembwever reviewed Oct 18, 2016

View reviewed changes

rustyrazorblade mentioned this pull request Oct 19, 2016

Cassandra Storage #3

Closed

adejanovski force-pushed the add-cassandra-storage branch 3 times, most recently from 2d03292 to a076cec Compare October 25, 2016 14:32

adejanovski force-pushed the add-cassandra-storage branch from a076cec to 56caf9b Compare October 28, 2016 09:40

michaelsembwever merged commit f68b8eb into master Nov 9, 2016

adejanovski deleted the add-cassandra-storage branch December 16, 2016 07:07

michaelsembwever unassigned rustyrazorblade Jul 24, 2017

michaelsembwever pushed a commit that referenced this pull request Apr 10, 2019

Merge pull request #2 from thelastpickle/mck/travis-older-cassandra-v…

bd3dbff

…ersions-integration-test Make the schema BasicSteps creates for integration tests compatible with Cassandra 1.2

Miles-Garnsey added a commit that referenced this pull request Aug 17, 2023

Add Management API agent for http tests (#2)

69abe66

Co-authored-by: Miles-Garnsey <miles.garnsey@datastax.com>

Miles-Garnsey added a commit that referenced this pull request Aug 17, 2023

Add Management API agent for http tests (#2)

7f8b52f

Co-authored-by: Miles-Garnsey <miles.garnsey@datastax.com>

Miles-Garnsey added a commit that referenced this pull request Aug 17, 2023

Add Management API agent for http tests (#2)

79844b3

Co-authored-by: Miles-Garnsey <miles.garnsey@datastax.com>

adejanovski pushed a commit that referenced this pull request Sep 6, 2023

Add Management API agent for http tests (#2)

82a29d6

Co-authored-by: Miles-Garnsey <miles.garnsey@datastax.com>

Miles-Garnsey added a commit that referenced this pull request Oct 3, 2023

Add Management API agent for http tests (#2)

d49c40b

Co-authored-by: Miles-Garnsey <miles.garnsey@datastax.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cassandra storage #2

Add cassandra storage #2

adejanovski commented Oct 17, 2016

michaelsembwever Oct 17, 2016

adejanovski Oct 20, 2016

michaelsembwever Oct 17, 2016

adejanovski Oct 20, 2016

michaelsembwever Oct 18, 2016

adejanovski Oct 20, 2016

michaelsembwever Oct 18, 2016

adejanovski Oct 20, 2016

michaelsembwever Oct 18, 2016

adejanovski Oct 20, 2016

michaelsembwever Oct 18, 2016

adejanovski Oct 20, 2016

michaelsembwever Oct 18, 2016

adejanovski Oct 20, 2016

michaelsembwever Oct 18, 2016

adejanovski Oct 20, 2016

michaelsembwever commented Oct 24, 2016

rustyrazorblade commented Oct 24, 2016

adejanovski commented Oct 25, 2016

michaelsembwever commented Oct 25, 2016

zznate commented Oct 27, 2016

adejanovski commented Oct 28, 2016

Add cassandra storage #2

Add cassandra storage #2

Conversation

adejanovski commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsembwever commented Oct 24, 2016

rustyrazorblade commented Oct 24, 2016

adejanovski commented Oct 25, 2016

michaelsembwever commented Oct 25, 2016

zznate commented Oct 27, 2016

adejanovski commented Oct 28, 2016