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

Feat: Introducing the GarbageCollector of local snapshots #995

Merged
merged 13 commits into from Oct 22, 2018

Conversation

Projects
None yet
4 participants
@hmoog
Copy link
Contributor

hmoog commented Sep 18, 2018

This PR contains the GarbageCollector and its corresponding package, that is used by local snapshots to clean up the database.

@jakubcech jakubcech referenced this pull request Sep 18, 2018

Closed

Review snapshot logic for local snapshots #980

5 of 6 tasks complete

@hmoog hmoog changed the title Merge garbage collector Feat: Introducing the GarbageCollector of local snapshots Sep 18, 2018

if (garbageCollectorJobs.get(jobClass) == null) {
synchronized(this) {
if (garbageCollectorJobs.get(jobClass) == null) {
garbageCollectorJobs.put(jobClass, new ArrayDeque<>());

This comment has been minimized.

@th0br0

th0br0 Sep 18, 2018

Contributor

This should be concurrent (it's single threaded right now anyway) just so a future parallelisation of this doesn't cause unexpected races.

This comment has been minimized.

@hmoog

hmoog Sep 18, 2018

Contributor

It's single threaded by design but what do you mean with concurrent? Instead of the ArrayDeque? because the getter is already thread safe by using double-checked locking.

Anyway here the reasons which i already posted in slack for doing this sequentially / single threaded:

  1. this is mostly IO bound so spawning additional threads would not increase the througput alot - in fact it could even be harmfull - let me comment stackoverflow "IO to a single physical disk is intrinsically a serialized operation. Each thread would have to block, waiting for its chance to get access to the disk. In this case, multiple threads are probably useless...and may even lead to contention problems."

  2. this is pretty much the least priority task in the node itself - we do not want to affect the overall performance of a node just to speed up the pruning (which due to 1. will not scale very well with the amount of workers anyway).

  3. it allows us to compact the queues alot reducing both the memory footprint and the required hdd space to persist the the queues (in a secnario where a big db file needs to be cleaned up) we only need 48bytes to represent the pruning state (compared to possibly hundreds or even gigabytes of data if we need to be able to randomly address any job in the queue to take note that we pruned him up already - at some point persisting the state (to be able to continue) will require much more resources then the pruning itself)

  4. since we need to do deletions atomicly in transactions because we otherwise loose the ability to address certain transactions if their connections to the tangle have been removed already and they are floating now) having multiple threads would also not be a good thing since it would increase the time it takes for 1 sub task to finish and therefore give IRI more chances to crash in between

  5. since the addition of jobs gets triggered by the snapshot taking routine - which needs to lock the snapshot state while it is taking the snapshot we want additions to happen as fast as possible
    so we can unlock the snapshots (which are the representation of the ledger state) as fast as possible without blocking the rest of the node
    so having scenarios where we need to synchronize the processing and changes of the queues would reduce the performance of snapshots being taken and therefore the performance of the whole node

@hmoog hmoog requested a review from karimodm Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 18, 2018

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Partial review

* It plans, manages and executes the cleanup jobs asynchronously in a separate thread so cleaning up does not affect
* the performance of the other tasks of the node.
*/
public class GarbageCollector {

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

So we don't get confused with Java GC, can you change the name?

This comment has been minimized.

@hmoog

hmoog Sep 23, 2018

Contributor

Any name suggestions?

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

TanglePruner?
(I am bad at names :-P)

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

I want to start using interfaces for classes that are not part of the domain model but execute business logic.
Currently this doesn't happen much in IRI but it should happen. We want to depend on abstractions not concretions (SOLID).

Just move all the public methods and javadocs to the interface please.
You can append "Impl" to the name of the implementing class.


/**
* Logger for this class allowing us to dump debug and status messages.
*/

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

I think this comment is extraneous. Not a big deal though if you leave it.
Just for the future you can save time on it.

This comment has been minimized.

@hmoog

hmoog Sep 20, 2018

Contributor

I created a live template for this so i don't have to "reinvent" this every time i comment a logger :P While I agree that some comments are self explanatory i still think its nice to have comments on all methods and properties (just because it feels nice to have a class completely documented).

Especially compared to the state IRI is in right now where nearly nothing is documented.

Also see my comment regarding the "Tangle" property.

/**
* Boolean flag that indicates if the node is being shutdown.
*/
boolean shuttingDown = false;

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

Can you make all the package-private fields private if possible?

This comment has been minimized.

@hmoog

hmoog Sep 20, 2018

Contributor

they are used by the jobs to check if the garbageCollector is still running - so the alternative would be a protected getter that would allow the jobs to request the status of the manager

/**
* Holds a reference to the tangle instance which acts as an interface to the used database.
*/
final Tangle tangle;

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

I usually prefer to not add a comment for fields that represent common services and just rely on the class javadocs (bad example here on my side since Tangle is missing javadocs).

The reason is that such references appear a lot throughout the code and you will just have to repeat the same comment everywhere and then if necessary change in lots of places.

What I do is to try to document everything that is public.
Documenting private is extra-credit :-)

This comment has been minimized.

@hmoog

hmoog Sep 20, 2018

Contributor

Sometimes 3rd party developers are interested in just certain parts of the code and don't want to digest the whole project to understand how things are working.

Having been a 3rd party developer myself not too long ago i would have really appreciated comments like that, that prevent me from having to analyze the Tangle class to understand what this thing is for (especially since "Tangle" it not really giving away what it is doing).

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

As a 3rd party developer didn't you usually use the docs hotkey on the IDE to display the docs without switching to the class?

I agree that if you look over the code on github or a simple editor it helps.
However, since the problem can easily be resolved via an IDE and I am a firm believer of DRY, I still think that such comments are code you have to maintain and change in lots of places (tangle exists in lots of class).

I personally want to add documentation to IRI classes I will refactor, but I will only add them to public members.
I am planning to add pmd rules that will make codacy shout out when devs don't comment public members.
I won't enforce other devs to comment on every little thing though.

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

btw,
This should be private. Let's add those getters. If for some reason we will need to remove the final in the future it will be much harder to refactor the code.
There are good reasons for most of the java conventions.
I'd rather follow them.

/**
* A map of {@link QueueProcessor}s allowing us to process queues based on the type of the job.
*/
private final HashMap<Class<? extends GarbageCollectorJob>, QueueProcessor> queueProcessors = new HashMap<>();

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

For example, this is a doc I like.
I don't know if I would personally bothered to document it since it is private, but I see it as a useful doc :-)


try {
Thread.sleep(GARBAGE_COLLECTOR_RESCAN_INTERVAL);
} catch(InterruptedException e) { /* do nothing */ }

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

I know this is how it looks like in the rest of the code.
But actually we are not supposed to ignore interruptedException.
See #605 (that was never merged due to me asking something that wasn't fulfilled), and https://dzone.com/articles/how-to-handle-the-interruptedexception

).<CharSequence>map(
entry -> entry.getClass().getCanonicalName() + ";" + entry.serialize()
).iterator()
);

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

Just a question (since I am not a functional type of guy), do you find this readable?
Maybe it is just me not being down with functional programming?

This comment has been minimized.

@hmoog

hmoog Sep 23, 2018

Contributor

Yes, this is how it usually look :D

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

Hmm, I am pretty sure since you're doing entry.getValue().stream() that you are not serializing what you want.
I may be wrong... since this method confused me due to not being too readable. I was also advised by a functional programming enthusiastic person in the office who agreed it is not readable.
Can you please make it readable?
Maybe extract some private methods and variables?

* @throws GarbageCollectorException if anything goes wrong while processing the cleanup jobs
*/
private void processCleanupJobs() throws GarbageCollectorException {
for(Map.Entry<Class<? extends GarbageCollectorJob>, ArrayDeque<GarbageCollectorJob>> entry : garbageCollectorJobs.entrySet()) {

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

ArrayDeque = Queue

This comment has been minimized.

@hmoog

hmoog Sep 23, 2018

Contributor

Since i need things like addLast and so on I kind of need to be specific here.

This comment has been minimized.

@GalRogozinski

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

hmm
Maybe you want to clone the entrySet with new HashSet<>(garbageCollectorJobs.entrySet())
The reason is that I am unsure whether another thread may modify garbageCollectorsJobs...

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

How do you release the jobs you already performed?


reader.close();
}
catch(IOException e) { /* do nothing */ }

This comment has been minimized.

@GalRogozinski

This comment has been minimized.

@hmoog

hmoog Sep 20, 2018

Contributor

Yes really :) Having local snapshot files is optional - especially the first time you launch IRI after we enable this feature there will simply be no local snapshot files and also no garbage collector state file - so this is not a critical error that needs to get handled separately.

}
}

reader.close();

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 20, 2018

Member

should be in finally or use try with parentheses (does auto-close).

@iotaledger iotaledger deleted a comment from codacy-bot Sep 23, 2018

@iotaledger iotaledger deleted a comment from hmoog Sep 23, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Sep 24, 2018

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Hey,

Takes me a long time to review and I still have some stuff to loose ends here I didn't finish looking at.

Before I press the approve button I want you to add unit tests.

One reason I am insisting on it now and not later is because I think your code is not very testable. For instance, if you want to fill in the job queue for a test you must also consolidate...

Anyhows, please write unit tests.

/**
* Holds a reference to the tangle instance which acts as an interface to the used database.
*/
final Tangle tangle;

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

As a 3rd party developer didn't you usually use the docs hotkey on the IDE to display the docs without switching to the class?

I agree that if you look over the code on github or a simple editor it helps.
However, since the problem can easily be resolved via an IDE and I am a firm believer of DRY, I still think that such comments are code you have to maintain and change in lots of places (tangle exists in lots of class).

I personally want to add documentation to IRI classes I will refactor, but I will only add them to public members.
I am planning to add pmd rules that will make codacy shout out when devs don't comment public members.
I won't enforce other devs to comment on every little thing though.

this.tipsViewModel = tipsViewModel;

MilestonePrunerJob.registerInGarbageCollector(this);
UnconfirmedSubtanglePrunerJob.registerInGarbageCollector(this);

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

What confused me is that I expected the static registerInGarbageCollector to add the garbage collector to some static container of ..PrunerJob.
However it invokes a method of registerParser which belongs to garbageCollector. So the garbageCollector itself is still the object making the "too many assumptions about the way the job works" in a way.
You just passed the responsibility of assigning this to ...PrunerJob class (not object).

Also it confuses me that parse in ..PrunerJob is private since the garbageCollector obviously invokes it. This breaks the encapsulation I am expecting.
It actually kinda astounded me than you can do that. You declared a public method via an interface than had it implemented by a private static. Kind of an awesome hack, but I have to reject it.

)
);

try {

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

Why do you have this try nested inside another try?

}
}
} finally {
reader.close();

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

this is fine but I think I want to enforce the use of try(reader) that will do an autoclose.

reader.close();
}
}
catch(IOException e) { /* do nothing */ }

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

Why are you ignoring this?
(I remember I asked this but I don't see a reply)

* It simply stores the provided parameters in its according protected properties.
*
* Since cleanup jobs can be consolidated (to reduce the size of the garbage collector state file), we need to be
* able provide both parameters even tho the job usually always "starts" with its {@code currentIndex} being equal

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

typo tho -> though

* Apart from offering method to spawn and terminate {@link Thread}s, it also offers methods to {@link #sleep(int)}
* while correctly maintaining the {@link Thread#isInterrupted()} flags.
*/
public class ThreadUtils {

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

It looks nice.
However I didn't mean for you to create something with global scope.

I thought about using maybe using Executors.newSingleThreadExecutor() with a custom ThreadFactory for naming. Sorry for not being explicit on the last review.

Having said that this implementation is quite nice. Due to the global scope you have a synchronized call though... It would have been avoided if you just used a solution with no global scope. Just for this reason I am asking you to change :-)

This comment has been minimized.

@hmoog

hmoog Oct 15, 2018

Contributor

The synchronized call is not there because of it being a "global" utility class but to prevent the spawning of multiple threads when "start" is being called multiple times. Please note that the synchronization happens over the threadIdentifier, so multiple classes can simultaneously use this class without having to wait for the synchronization.

so if a class implements this functionality:

private ThreadIdentifier myIdentifier = new ThreadIdentifier("Thread Name for console");

...

public void start() {
ThreadUtils.spawnThread(this::myThreadMethod, myIdentifier);
}

and i call

myClassInstance.start();
myClassInstance.start();
myClassInstance.start(); // from different threads

it still only spawns one thread

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

This will be fixed in #1061


return false;
}
}

This comment has been minimized.

@GalRogozinski
).<CharSequence>map(
entry -> entry.getClass().getCanonicalName() + ";" + entry.serialize()
).iterator()
);

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

Hmm, I am pretty sure since you're doing entry.getValue().stream() that you are not serializing what you want.
I may be wrong... since this method confused me due to not being too readable. I was also advised by a functional programming enthusiastic person in the office who agreed it is not readable.
Can you please make it readable?
Maybe extract some private methods and variables?

* @param job the job that shall be executed
* @throws GarbageCollectorException if anything goes wrong while adding the job
*/
public void addJob(GarbageCollectorJob job) throws GarbageCollectorException {

This comment has been minimized.

@GalRogozinski

GalRogozinski Sep 25, 2018

Member

maybe change name to addJobAndConsolidate?

This comment has been minimized.

@hmoog

hmoog Sep 26, 2018

Contributor

This would imply that there would also be a method for just adding without consolidation.

The consolidation is an internal mechanism of the Manager to reduce its memory and disk consumption - as the user of the Manager i don't care about how it performs its task - i just wannt to hand over the tasks and that's it. Maybe it get's a bit clearer if we seperate the interfaces from their implementation.

@iotaledger iotaledger deleted a comment from codacy-bot Oct 16, 2018

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Looks a whole lot better!
I think I actually understood how it works now :-)

I was a little annoyed by the cyclic dependency between the TransactionPruner and the Job but it is not so bad.

Still has a few rejects.

Now what is important has been labeled as a bug in the comment. The rest are comments. Some are important, some are nits (like comments I have on the javadocs).

* @param job the job that shall be executed
* @throws TransactionPruningException if anything goes wrong while adding the job
*/
void addJob(TransactionPrunerJob job) throws TransactionPruningException;

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

Like on the custom exception


/**
* This class implements the contract of the {@link TransactionPruner} while executing the jobs asynchronously in the
* background.

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

When you view the generated javadocs you see:

public class AsyncTransactionPruner
extends Object
implements TransactionPruner

With links.

Thus this is a bit extraneous:

This class implements the contract of the {@link TransactionPruner}

This comment has been minimized.

@hmoog

hmoog Oct 18, 2018

Contributor

I kind of have to start the sentence somehow, when i try to say that it adds extra funtionality to the basic contract :P

@Override
public void processJobs() throws TransactionPruningException {
for(JobQueue jobQueue : jobQueues.values()) {
if(Thread.interrupted()) {

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

Just to be clear that it is by design, You clear the interrupted status so you just skip this one job but keep on processing the others?

This comment has been minimized.

@hmoog

hmoog Oct 18, 2018

Contributor

Why do i continue processing the others? I return from the whole method.

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 21, 2018

Member

From the javadocs of Thread.interrupted:

Tests whether the current thread has been interrupted. The interrupted status of the thread is cleared by this method. In other words, if this method were to be called twice in succession, the second call would return false (unless the current thread were interrupted again, after the first call had cleared its interrupted status and before the second call had examined it).

Now you do return indeed from the method. However processJobsThread() will call processJobs again... Then you will start iterating over the job queue again (so I was wrong you won't skip anything).

You can use Thread.currentThread().isInterrupted() to not clear the flag.

while ((line = reader.readLine()) != null) {
String[] parts = line.split(";", 2);
if (parts.length >= 2) {
JobParser jobParser = this.jobParsers.get(parts[0]);

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

We don't really use this keyword (we rely on syntax highlighting). This is just how it was mostly done until now.
This is a style comment that should be automated...

This comment has been minimized.

@hmoog

hmoog Oct 18, 2018

Contributor

okay that is unnecessary and i usually dont use it - must have slipped through

}
} catch(IOException e) {
if(getStateFile().exists()) {
throw new TransactionPruningException("could not read the state file", e);

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

So we swallow the exception if the if fails? -> bug

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

Plus you are missing spaces after if and catch (style comment that should be automated)

This comment has been minimized.

@hmoog

hmoog Oct 18, 2018

Contributor

its not a bug - its intended - if no statefile exists then there is nothing to restore and we can ignore it

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 22, 2018

Member

I see. It is confusing/misleading that the normal logic of the method is being handled in a catch block. If you are still correcting the code, could you take this if out of the catch block.

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 22, 2018

Member

I see. It is confusing/misleading that the normal logic of the method is being handled in a catch block. If you are still correcting the code, could you take this if out of the catch block.

}

/**
* Does same as {@link #MilestonePrunerJob(int, int)} but defaults to the {@link #currentIndex} being the same as

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

you're missing an int in the javadocs

This comment has been minimized.

@hmoog

hmoog Oct 18, 2018

Contributor

true - will fix


setCurrentIndex(getCurrentIndex() + 1);

// synchronize this call because the MilestonePrunerJobQueue needs it to check if we can be extended

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

great comment :-)


// persist changes if the job was executed by a TransactionPruner (tests might run it standalone)
if (getTransactionPruner() != null) {
getTransactionPruner().saveState();

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

You already do that in ProcessJobs. Please delete -> bug

Regardless of the bug, I think it is a bad design to hide an important functionality in a method like this.
The way you did it in ProcessJobs is good. If you must put it in process it would have been better if you would have added a boolean flag in the parameters that will direct if we want the save functionality.

This comment has been minimized.

@hmoog

hmoog Oct 18, 2018

Contributor

its not a bug - the MilestonePrunerJob will run a very long time and process milestones one at a time - everytime it finishes one of these subtasks it persists its progress. I dont want to only save the progress once the whole job is done but whenever it finish cleaning up one of the milestones.

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 21, 2018

Member

hmm:

  1. So in this case isn't the outer saveState is extraneous. Because once you are done you will still save state in the inner saveState and then once more in the outer. I suppose for other types of jobs you have to do it though...
    If I understood right one way to overcome the above issue is to add isSavingStateWhileProcessing (or something with shorter name) that will return a boolean to TransactionPrunerJob. Then you can omit the extra save in the end.

  2. There is a slight design problem that process implicitly saves state but that is too hard to fix now I think so I will just ignore it. Just FYI

  3. I don't like the test "combina" (http://www.ehebrew.org/articles/hebrew-slang.php#.W8yg4XVubdE). In test the job will have to receive a mock or else get an NPE.

This comment has been minimized.

@hmoog

hmoog Oct 21, 2018

Contributor

No the outer saveState is not extraneous because once the job is DONE it will get removed from the Queue and we also need to "save" this. This can not be done from the "inside" because the job doesn't know it is managed by a queue and if and when it will get removed from the Queue.

I am okay with removing this null check and rely on a mock for tests.

} catch(Exception e) {
throw new TransactionPruningException("failed to cleanup milestone #" + getCurrentIndex(), e);
}
}

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

Funny that codacy didn't report that this method is too complex.
It is not that bad but I would break it up to private method.
If you decide to do that than make sure the first private method name is explicit about creating pruner jobs.

This comment has been minimized.

@hmoog

hmoog Oct 21, 2018

Contributor

okay i will refactor it and break it down to another private method

* Apart from offering method to spawn and terminate {@link Thread}s, it also offers methods to {@link #sleep(int)}
* while correctly maintaining the {@link Thread#isInterrupted()} flags.
*/
public class ThreadUtils {

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 18, 2018

Member

This will be fixed in #1061

@hmoog hmoog closed this Oct 22, 2018

@hmoog hmoog reopened this Oct 22, 2018

@hmoog hmoog closed this Oct 22, 2018

@hmoog hmoog reopened this Oct 22, 2018

@@ -25,7 +26,7 @@
* @see #spawnThread(Runnable, ThreadIdentifier)
* @see #stopThread(ThreadIdentifier)
*/
private static final HashMap<Object, Thread> threads = new HashMap<>();
private static final Map<Object, Thread> threads = new HashMap<>();

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 22, 2018

Member

I thought we will get rid of this class in later PR.
Of course I don't object to the change.

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Excellent

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@@ -0,0 +1,15 @@
package com.iota.iri.service.snapshot;

import com.iota.iri.conf.MainnetConfig;

This comment has been minimized.

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Oct 22, 2018

@GalRogozinski GalRogozinski merged commit 4ed2a7e into iotaledger:dev Oct 22, 2018

1 check failed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details

@hmoog hmoog deleted the iotadevelopment:merge_garbageCollector branch Nov 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment