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

Fresh transactionToRequest #1316

Merged
merged 11 commits into from Feb 4, 2019

Conversation

Projects
None yet
3 participants
@karimodm
Copy link
Member

karimodm commented Jan 30, 2019

Description

Keep the transactionToRequest Set of the TransactionRequester fresh with new inbound transactions instead of keeping stale ones when at capacity.

Fixes #1313

Type of change

  • Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

  • Unsynced node that lag greatly behind seem to sync faster since we do not pile up stale transations anymore.

Checklist:

  • My code follows the style guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

karimodm added some commits Jan 30, 2019

Remove from transactionsToRequest only when received
This will allow the Set of transactionsToRequest to maintain its
ordering, in order to be able to remove oldest transactions.

@karimodm karimodm requested review from GalRogozinski and alon-e Jan 30, 2019

@karimodm karimodm changed the title 1313 fresh transactionstorequest Fresh transactionToRequest Jan 30, 2019

@karimodm karimodm force-pushed the karimodm:1313-fresh-transactionstorequest branch from b655341 to f8921a4 Jan 30, 2019

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Looks good

@GalRogozinski

This comment has been minimized.

Copy link
Member

GalRogozinski commented Jan 31, 2019

@karimodm
I approved but maybe add a unit test that checks this very specific change
If you feel like adding other unit tests that don't concern your change do it in another PR so we don't delay the merger of this one

@karimodm

This comment has been minimized.

Copy link
Member Author

karimodm commented Jan 31, 2019

@GalRogozinski wrote tests.

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Good Job :-)

I have 2 suggestions that you can opt to accept or decline.
Even if you decline them, just be aware of them for next time

Show resolved Hide resolved src/test/java/com/iota/iri/network/TransactionRequesterTest.java Outdated
TransactionViewModelTest.getRandomTransactionHash()
));
TransactionRequester txReq = new TransactionRequester(tangle, snapshotProvider, mq);
int capacity = TransactionRequester.MAX_TX_REQ_QUEUE_SIZE;

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 31, 2019

Member

There is a thumb rule that we should declare a local variable as soon as it is being used.
Not going to insist on it now, just that you know for next time.

Change it only if you feel like it

This comment has been minimized.

@karimodm

karimodm Feb 1, 2019

Author Member

This comes from my habit to C. In C you should always declare local variable at the beginning of the function, just for the compiler to know how much to shift the stack base pointer: it used to result in a warning =)

This comment has been minimized.

@GalRogozinski

GalRogozinski Feb 4, 2019

Member

Since in java we have a heavily optimized compiler readability comes first
For next time :-)

txReq.requestTransaction(eldest, false);
txReq.requestTransaction(TransactionViewModelTest.getRandomTransactionHash(), false);
txReq.requestTransaction(TransactionViewModelTest.getRandomTransactionHash(), false);
txReq.requestTransaction(TransactionViewModelTest.getRandomTransactionHash(), false);

This comment has been minimized.

@GalRogozinski

GalRogozinski Jan 31, 2019

Member

Just heads up

Technically it is not a good idea to use randomness in unit tests (you want them to be consistent)
Unfortunately, the current codebase encourages you to write this way.

For this specific unit test it is less of a problem and I don't want to currently fix this problem which is not related to you or the task at hand.

Just FYI

This comment has been minimized.

@karimodm

karimodm Feb 1, 2019

Author Member

I thought the same when I was coding but well...

GalRogozinski and others added some commits Feb 1, 2019

Use List interface instead of ArrayList
Co-Authored-By: karimodm <andreakarimodm@gmail.com>
Merge remote-tracking branch 'refs/remotes/origin/1313-fresh-transact…
…ionstorequest' into 1313-fresh-transactionstorequest

@iotaledger iotaledger deleted a comment from codacy-bot Feb 1, 2019

@iotaledger iotaledger deleted a comment from codacy-bot Feb 1, 2019

@iotaledger iotaledger deleted a comment from codacy-bot Feb 1, 2019

@iotaledger iotaledger deleted a comment from codacy-bot Feb 1, 2019

TransactionViewModelTest.getRandomTransactionHash()
));
TransactionRequester txReq = new TransactionRequester(tangle, snapshotProvider, mq);
int capacity = TransactionRequester.MAX_TX_REQ_QUEUE_SIZE;

This comment has been minimized.

@GalRogozinski

GalRogozinski Feb 4, 2019

Member

Since in java we have a heavily optimized compiler readability comes first
For next time :-)

Show resolved Hide resolved src/test/java/com/iota/iri/network/TransactionRequesterTest.java Outdated
Show resolved Hide resolved src/test/java/com/iota/iri/network/TransactionRequesterTest.java Outdated

GalRogozinski and others added some commits Feb 4, 2019

Update src/test/java/com/iota/iri/network/TransactionRequesterTest.java
Co-Authored-By: karimodm <andreakarimodm@gmail.com>
Update src/test/java/com/iota/iri/network/TransactionRequesterTest.java
Co-Authored-By: karimodm <andreakarimodm@gmail.com>

@GalRogozinski GalRogozinski merged commit 95d38a2 into iotaledger:dev Feb 4, 2019

6 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
buildkite/iri-build-jar-prs Build #409 passed (9 minutes, 16 seconds)
Details
buildkite/iri-build-jar-prs/8e85da56-a1e5-41c0-bbbd-89c9b15d7bcb Passed (6 minutes, 6 seconds)
Details
buildkite/iri-build-jar-prs/build-iri-oracle8 Passed (2 minutes, 40 seconds)
Details
buildkite/iri-build-jar-prs/pull-from-repo Passed (17 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment