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: Fixes + Improvements for IRI that are required for local snapshots #1095

Merged
merged 11 commits into from Nov 1, 2018

Conversation

Projects
None yet
3 participants
@hmoog
Copy link
Contributor

hmoog commented Oct 24, 2018

This PR contains a few fixes and improvements for IRI that are not directly related to local snapshots but that have been identified during its development and that are required for the activation of this feature later on.

@hmoog hmoog requested review from GalRogozinski and karimodm Oct 24, 2018

@@ -103,6 +113,7 @@ public void init() throws Exception {
parseNeighborsConfig();

executor.submit(spawnBroadcasterThread());
executor.submit(spawnRequesterThread());
executor.submit(spawnTipRequesterThread());

This comment has been minimized.

@gjeee

gjeee Oct 24, 2018

Contributor

would it be an idea to rename spawnTipRequesterThread() to spawnLatestMilestoneRequesterThread() for clarity (and adjust the naming in the log statements as well) ?

This comment has been minimized.

@hmoog

hmoog Oct 24, 2018

Contributor

It's not a LatestMilestoneRequester - the tip requester sends the hash of the milestone (request part) along with the data of that milestone (data part).

If we send a transaction that has a hash in its request part that matches the transaction in the data part, the receiving node interprets it as "i don't need anything - please send me a random tip".

So what this thread does is regularly requesting a random tip from its neighbours.

This comment has been minimized.

@gjeee

gjeee Oct 24, 2018

Contributor

aha now i understand this. so it serves actually two purposes:

  1. broadcasting latest milestone info which could be empty object (data part)
  2. doing a random tip request (request part)

so one could call it either spawnLatestMilestoneBroadcasterThread or spawnRandomTipRequesterThread. Both are equally valid I guess.

thanks for clearing this up.....i was wondering about this construct in issue #1054 as well.

@@ -103,6 +113,7 @@ public void init() throws Exception {
parseNeighborsConfig();

executor.submit(spawnBroadcasterThread());
executor.submit(spawnRequesterThread());

This comment has been minimized.

@gjeee

gjeee Oct 24, 2018

Contributor

maybe spawnMissingTxsRequester() would be better name

This comment has been minimized.

@hmoog

hmoog Oct 24, 2018

Contributor

Its the thread that works through the requestQueue rather than specifically requesting selective transactions - dunno if another name would make it easier to understand - but i don't really mind how its called as long as we remove the limitation to only allow transactions to be requested when we broadcast transactions.

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

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

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

change requests just for the isSnapshot name and adding toString() to Snapshot

*
* @return human readable string representation of the milestone
*/
@Override

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 31, 2018

Member

You don't really need custom docs for toString() you can simply inherit it :-)
Not that you need to change it. I just think it is a waste of time to document well known pre-documented methods.

However this reminds me:
You didn't add toString() to Snapshot, SnapshotState and SnapshotData

This comment has been minimized.

@hmoog

hmoog Oct 31, 2018

Contributor

will take a node and add it in a separate PR

* @param isSnapshot true if the transaction is a snapshot and false otherwise
* @throws Exception if something goes wrong while saving the changes to the database
*/
public void isSnapshot(Tangle tangle, final boolean isSnapshot) throws Exception {

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 31, 2018

Member

Change to setLocalSnapshot parameter name should be localSnapshot (java is typed so it is fine).
final here is just extra verbosity

public void isSnapshot(Tangle tangle, final boolean isSnapshot) throws Exception {
if (isSnapshot != transaction.isSnapshot) {
transaction.isSnapshot = isSnapshot;
update(tangle, "isSnapshot");

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 31, 2018

Member

"localSnapshot"

/**
* This flag indicates if the transaction is a coordinator issued milestone.
*/
public boolean isSnapshot = false;

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 31, 2018

Member

localSnapshot

since we don't change snapshot ...

This comment has been minimized.

@hmoog

hmoog Oct 31, 2018

Contributor

actually this should in fact be isMilestone - i just kept the old naming scheme (snapshotIndex) O_O

@@ -52,7 +62,7 @@
private final DatagramPacket sendingPacket;
private final DatagramPacket tipRequestingPacket;

private final ExecutorService executor = Executors.newFixedThreadPool(5);
private final ExecutorService executor = Executors.newFixedThreadPool(6);

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 31, 2018

Member

why id you change this?

*
* @return lambda for the requester {@link Thread}
*/
private Runnable spawnRequesterThread() {

This comment has been minimized.

@GalRogozinski

GalRogozinski Oct 31, 2018

Member

Maybe open an issue about doing something a bit more efficient.
Instead of having an infinite while loop maybe you can initiate this thread once the threshold has been exceeded.

@@ -69,7 +69,7 @@ private Runnable spawnReceiverThread() {

while (!shuttingDown.get()) {

if (((processed + dropped) % 50000 == 0)) {
if (((processed + dropped) % 50000 == 49999)) {

This comment has been minimized.

@hmoog

hmoog Oct 31, 2018

Contributor

to not spam the console output when there are 0 processed packets

@@ -336,7 +336,7 @@ public void deleteBatch(Collection<Pair<Indexable, ? extends Class<? extends Per
ColumnFamilyHandle handle = classTreeMap.get(entry.hi);
writeBatch.remove(handle, keyBytes);
ColumnFamilyHandle metadataHandle = metadataReference.get(entry.hi);
if (metadataReference != null) {
if (metadataHandle != null) {

This comment has been minimized.

@hmoog

hmoog Oct 31, 2018

Contributor

its just wrong :P

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Just one small quick little change.
Sorry but I am not the one who made up the java convention rules.

@GalRogozinski GalRogozinski merged commit 0e561af into iotaledger:dev Nov 1, 2018

1 of 2 checks passed

Codacy/PR Quality Review Hang in there, Codacy is reviewing your Pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

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