Use worker cleanup task instead of direct jedis cleanup #89

rajatparida86 · 2019-01-07T16:50:10Z

Relates to #87

alexkorotkikh · 2019-01-08T07:28:44Z

src/main/java/de/zalando/zmon/scheduler/ng/CommandSerializer.java

+    public byte[] writeCleanUp(Set<String> removedIds){
+        CeleryBody body = new CeleryBody();
+
+        body.task= "cleanup";


I know that we don't have a linter for java projects yet, but what do you think if we'll try to follow some basic formatting rules, common for most languages? Like spaces before and after = and + for example.

alexkorotkikh · 2019-01-08T07:31:12Z

src/main/java/de/zalando/zmon/scheduler/ng/entities/EntityRepository.java

@@ -24,6 +25,9 @@
 @Component
 public class EntityRepository extends CachedRepository<String, EntityAdapterRegistry, Entity> {

+    @Autowired
+    Scheduler scheduler;


Best practice is to use constructor injection over field or setter injection (e.g. https://www.vojtechruzicka.com/field-dependency-injection-considered-harmful/)

rajatparida86 · 2019-01-08T12:01:55Z

@alexkorotkikh could you review please?

codecov · 2019-01-08T12:03:35Z

Codecov Report

Merging #89 into master will increase coverage by 0.67%.
The diff coverage is 50%.

@@             Coverage Diff              @@
##             master      #89      +/-   ##
============================================
+ Coverage     19.95%   20.63%   +0.67%     
- Complexity      152      159       +7     
============================================
  Files            92       94       +2     
  Lines          2290     2331      +41     
  Branches        232      234       +2     
============================================
+ Hits            457      481      +24     
- Misses         1795     1812      +17     
  Partials         38       38

Impacted Files	Coverage Δ	Complexity Δ
...alando/zmon/scheduler/ng/DataCenterSubscriber.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../zmon/scheduler/ng/cleanup/BatchEntityCleanup.java	`0% <0%> (ø)`	`0 <0> (?)`
...mon/scheduler/ng/cleanup/EntityChangedCleaner.java	`65.62% <0%> (-2.12%)`	`3 <0> (ø)`
...zmon/scheduler/ng/cleanup/SingleEntityCleanup.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ler/ng/config/BatchEntityCleanupConfiguration.java	`0% <0%> (ø)`	`0 <0> (?)`
.../java/de/zalando/zmon/scheduler/ng/CeleryBody.java	`66.66% <100%> (+2.38%)`	`1 <0> (ø)`	⬇️
...zalando/zmon/scheduler/ng/scheduler/Scheduler.java	`48.62% <100%> (+1.45%)`	`12 <1> (+1)`	⬆️
...e/zalando/zmon/scheduler/ng/CommandSerializer.java	`54.87% <78.94%> (+7.73%)`	`4 <2> (+1)`	⬆️
...o/zmon/scheduler/ng/entities/EntityRepository.java	`60.83% <83.33%> (+1.18%)`	`26 <0> (+3)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d41f40...5802283. Read the comment docs.

alexkorotkikh

👍

see also zalando-zmon/zmon-scheduler#89

alexkorotkikh · 2019-01-09T08:04:54Z

src/main/java/de/zalando/zmon/scheduler/ng/CeleryBody.java

@@ -65,6 +65,10 @@
        public int priority = 1;
        public String team;
    }
+
+    public static class EntityCleanUpArg{


EntityCleanUpArg<space>{

alexkorotkikh · 2019-01-09T08:07:16Z

src/main/java/de/zalando/zmon/scheduler/ng/CommandSerializer.java

@@ -24,21 +21,43 @@ public CommandSerializer(TaskSerializerType type, Tracer tracer) {
    }

    public String expiresTime(long interval) {
-        Date exp = new Date(System.currentTimeMillis()+(interval * 1000L));
+        Date exp = new Date(System.currentTimeMillis() + (interval * 1000L));


it's error-prone to have interval as long. E.g. if interval == Long.MAX_VALUE, what do you expect interval * 1000 to be equal to? I'd change interval to int and maybe rename it some way to clarify it's in seconds...

@alexkorotkikh this piece of code was previously there. Only formatting changes were introduced by me. I understand your point. However, I would solve this in another PR rather than using this PR to solve it

oh, I was inattentive. Sorry about that.

alexkorotkikh · 2019-01-09T08:08:27Z

src/main/java/de/zalando/zmon/scheduler/ng/cleanup/BatchEntityCleanup.java

+    public void notifyEntityRemove(EntityRepository repo, Entity e) {
+    }
+
+    public BatchEntityCleanup(Scheduler scheduler) {


constructor usually goes before methods in class declaration

alexkorotkikh · 2019-01-09T08:09:06Z

src/main/java/de/zalando/zmon/scheduler/ng/cleanup/SingleEntityCleanup.java

@@ -27,6 +30,9 @@
 */
 public class SingleEntityCleanup implements EntityChangeListener{


EntityChangeListener<space>{

alexkorotkikh · 2019-01-09T08:10:29Z

src/main/java/de/zalando/zmon/scheduler/ng/cleanup/SingleEntityCleanup.java

@@ -56,6 +62,9 @@ public void notifyEntityRemove(EntityRepository repo, Entity e) {
        executor.schedule(new EntityCleanupTask(e), 300, TimeUnit.SECONDS);
    }

+    @Override
+    public void notifyBatchEntityRemove (EntityRepository repo, Set<String> removedEntities) {}


I'm not sure, but it sounds semantically incorrect to have batch operation method in a class called SingleEntityCleanup. Did you think about introducing new Listener interface? Like BatchEntityChangeListener

The EntityRepository only allows listeners of type EntityChangeListener https://github.com/zalando-zmon/zmon-scheduler/blob/master/src/main/java/de/zalando/zmon/scheduler/ng/entities/EntityRepository.java#L31

alexkorotkikh · 2019-01-09T08:12:54Z

src/test/java/de/zalando/zmon/scheduler/ng/entities/EntityRepositoryTest.java

@@ -1,7 +1,10 @@
 package de.zalando.zmon.scheduler.ng.entities;

+import com.codahale.metrics.MetricRegistry;


Why were these imports added?

alexkorotkikh

👍

alexkorotkikh · 2019-01-24T07:21:59Z

src/test/java/de/zalando/zmon/scheduler/ng/entities/EntityRepositoryTest.java

        verify(listener).notifyEntityChange(eq(repository), eq(host1), eq(host1_changed));
    }
+
+    @Test
+    public void TestNotifyGlobalScheduler() {


It's not typical in java for method names to start from capital case

* cleanup entities task see also zalando-zmon/zmon-scheduler#89 * finish span... * fix span? * fix span? * log_kv requires dict

rajatparida86 · 2019-04-12T12:04:02Z

👍

mohabusama · 2019-04-12T12:53:12Z

👍

Use worker task instead of jedis cleanup

70332de

rajatparida86 added the DO NOT MERGE label Jan 7, 2019

rajatparida86 changed the title ~~Use worker task instead of jedis cleanup~~ Use worker cleanup task instead of direct jedis cleanup Jan 7, 2019

alexkorotkikh suggested changes Jan 8, 2019

View reviewed changes

Use appropriate UUID, Constructor injection, Formatting

395fd41

alexkorotkikh approved these changes Jan 8, 2019

View reviewed changes

rajatparida86 added 5 commits January 8, 2019 16:04

Separate interface method for batch entity cleanup

694d3e1

Batch entity cleanup

cd656a9

Add timelimit to redis payload

3ef0d03

Only cleanup if there is something to cleanup

9341fb7

Use TR as the prefix of task ID

de70fb8

vetinari added a commit to zalando-zmon/zmon-worker that referenced this pull request Jan 9, 2019

cleanup entities task

4128a3a

see also zalando-zmon/zmon-scheduler#89

vetinari mentioned this pull request Jan 9, 2019

cleanup entities task zalando-zmon/zmon-worker#388

Merged

alexkorotkikh suggested changes Jan 9, 2019

View reviewed changes

rajatparida86 added 5 commits January 9, 2019 10:21

Review comments

d0fd7e0

Only execute batch cleanup in remote schedulers

4228868

Log entity counts

886ba7b

Differentiate between Global and Local scheduler

6f5c0a7

unit tests for cleanup and global scheduler

ea90f38

alexkorotkikh approved these changes Jan 24, 2019

View reviewed changes

rajatparida86 added the in progress label Jan 24, 2019

rajatparida86 mentioned this pull request Jan 24, 2019

Entities in alert are not cleaned up in some cases #87

Open

mohabusama pushed a commit to zalando-zmon/zmon-worker that referenced this pull request Apr 5, 2019

cleanup entities task (#388)

6efff55

* cleanup entities task see also zalando-zmon/zmon-scheduler#89 * finish span... * fix span? * fix span? * log_kv requires dict

rajatparida86 added 2 commits April 12, 2019 13:58

Fix test method name to camel case

8537001

Fix camel case for missed method

5802283

rajatparida86 removed DO NOT MERGE in progress labels Apr 12, 2019

rajatparida86 merged commit e056554 into master Jun 26, 2019

pitr deleted the entity_cleanup branch June 27, 2019 14:40

vetinari restored the entity_cleanup branch July 3, 2019 11:03

vetinari mentioned this pull request Jul 3, 2019

Revert "Use worker cleanup task instead of direct jedis cleanup" #93

Merged

pitr deleted the entity_cleanup branch October 1, 2019 12:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use worker cleanup task instead of direct jedis cleanup #89

Use worker cleanup task instead of direct jedis cleanup #89

rajatparida86 commented Jan 7, 2019

alexkorotkikh Jan 8, 2019

rajatparida86 Jan 8, 2019

alexkorotkikh Jan 8, 2019

rajatparida86 commented Jan 8, 2019

codecov bot commented Jan 8, 2019 •

edited

Loading

alexkorotkikh left a comment

alexkorotkikh Jan 9, 2019 •

edited

Loading

alexkorotkikh Jan 9, 2019

rajatparida86 Jan 9, 2019

alexkorotkikh Jan 9, 2019

alexkorotkikh Jan 9, 2019

alexkorotkikh Jan 9, 2019 •

edited

Loading

alexkorotkikh Jan 9, 2019

rajatparida86 Jan 9, 2019

alexkorotkikh Jan 9, 2019

alexkorotkikh left a comment

alexkorotkikh Jan 24, 2019

rajatparida86 commented Apr 12, 2019

mohabusama commented Apr 12, 2019

		@@ -27,6 +30,9 @@
		*/
		public class SingleEntityCleanup implements EntityChangeListener{

		@@ -1,7 +1,10 @@
		package de.zalando.zmon.scheduler.ng.entities;

		import com.codahale.metrics.MetricRegistry;

Use worker cleanup task instead of direct jedis cleanup #89

Use worker cleanup task instead of direct jedis cleanup #89

Conversation

rajatparida86 commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatparida86 commented Jan 8, 2019

codecov bot commented Jan 8, 2019 • edited Loading

Codecov Report

alexkorotkikh left a comment

Choose a reason for hiding this comment

alexkorotkikh Jan 9, 2019 • edited Loading

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

alexkorotkikh Jan 9, 2019 • edited Loading

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

alexkorotkikh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatparida86 commented Apr 12, 2019

mohabusama commented Apr 12, 2019

codecov bot commented Jan 8, 2019 •

edited

Loading

alexkorotkikh Jan 9, 2019 •

edited

Loading

alexkorotkikh Jan 9, 2019 •

edited

Loading