yhassanzadeh13/skipgraph-java

An open-source implementation of Skip Graph DHT Overlay in Java.

skipgraph-java's Issues

[Testing] Add `ConcurrentLookupTableTest` tests

[Lint] Checkstyle the code

Context:

To run Checkstyle (currently available through make lint), and resolve all errors/warnings it finds.

How to proceed:

• Create a branch <yourname>/<issue-number>-<issue-name>
• Run make lint, and one by one solve the reported errors and warnings, run all tests through mvn test after each change to make sure that lint fixes are not breaking changes.
• The test should pass without making any change to its original implementation.

Definition of Done:

• Checkstyle issues have been resolved across the entire repo, passes, and the PR passes CI integration.

[Logger] Adding dynamic appender

Context:

Currently, we need to define the log level and add all contextual key-value pairs at the time we define the log.
example-1

 logger.fatal().
          Exception(e).
          Int("owner_num_id", owner.getNumID()).
          Int("num_id", node.getNumID()).
          Int("level", level).
          Int("sleep_time", sleepTime).
          Int("trial", trial).
          Msg("could not backoff");

This poses a redundancy issue where we need to log the same key values every time we log within the same context, e.g., for another logging attempt, we need to start putting together all key-value pairs:

example-2

 logger.debug().
          Int("owner_num_id", owner.getNumID()).
          Int("num_id", node.getNumID()).
          Int("level", level).
          Int("idx", idx).
          Msg("adding right node");

Solution:

Taking a closer look, there are some common key-value pairs between example-1 and example-2: owner_num_id, num_id, and level.

So, we can define a dynamic log appender that provides a loose-end appender that can be utilized to either append more pairs or append log message. We create the dynamic appender once at the beginning of the method:

DynamicLogAppender lg = logger.With().
                                                          Int("owner_num_id", owner.getNumID()).
                                                          Int("num_id", node.getNumID()).
                                                          Int("level", level).
                                                          DynamicAppender();

Then we can refactor our example-1 and -2 as following:

example-1:

 lg.fatal().
          Exception(e).
          Int("sleep_time", sleepTime).
          Int("trial", trial).
          Msg("could not backoff");

example-2:

 lg.debug().
          Int("idx", idx).
          Msg("adding right node");

Note-1: we should not determine the log level at the creation of a dynamic log appender, hence make it adaptable with any log level.

Note-2: note that we use the DynamicLogAppender, lg to manifest the log instead of logger.

[Logging] Refactor to loggerone

skipgraphnode package.

[Error Handling] Replace `system.err` calls with fatal-level logs

[Bug] Cannot read field "locked" because "response" is null

When increasing the number of nodes in the SkipNode tests, some tests (e.g., testSearchByMembershipVectorSequential) are failing due to the following exception. This bug happens at the 100 number of nodes. We temporarily set the number of nodes to 20 till we fix this issue. Most probably an insertion issue.

java.lang.NullPointerException: Cannot read field "locked" because "response" is null

	at middlelayer.MiddleLayer.send(MiddleLayer.java:81)
	at middlelayer.MiddleLayer.searchByIdentifier(MiddleLayer.java:277)
	at skipnode.SkipNode.searchByNumId(SkipNode.java:375)
	at middlelayer.MiddleLayer.receive(MiddleLayer.java:124)
	at underlay.Underlay.dispatchRequest(Underlay.java:53)
	at unittest.NetworkHub.deliver(NetworkHub.java:24)
	at unittest.MockUnderlay.sendMessage(MockUnderlay.java:28)
	at middlelayer.MiddleLayer.send(MiddleLayer.java:79)
	at middlelayer.MiddleLayer.searchByIdentifier(MiddleLayer.java:277)
	at skipnode.SkipNode.searchByNumId(SkipNode.java:375)
	at middlelayer.MiddleLayer.receive(MiddleLayer.java:124)
	at underlay.Underlay.dispatchRequest(Underlay.java:53)
	at unittest.NetworkHub.deliver(NetworkHub.java:24)
	at unittest.MockUnderlay.sendMessage(MockUnderlay.java:28)
	at middlelayer.MiddleLayer.send(MiddleLayer.java:79)
	at middlelayer.MiddleLayer.searchByIdentifier(MiddleLayer.java:277)
	at skipnode.SkipNode.searchByNumId(SkipNode.java:375)
	at middlelayer.MiddleLayer.receive(MiddleLayer.java:124)
	at underlay.Underlay.dispatchRequest(Underlay.java:53)
	at unittest.NetworkHub.deliver(NetworkHub.java:24)
	at unittest.MockUnderlay.sendMessage(MockUnderlay.java:28)
	at middlelayer.MiddleLayer.send(MiddleLayer.java:79)
	at middlelayer.MiddleLayer.searchByIdentifier(MiddleLayer.java:277)
	at skipnode.SkipNode.searchByNumId(SkipNode.java:375)
	at middlelayer.MiddleLayer.receive(MiddleLayer.java:124)
	at underlay.Underlay.dispatchRequest(Underlay.java:53)
	at unittest.NetworkHub.deliver(NetworkHub.java:24)
	at unittest.MockUnderlay.sendMessage(MockUnderlay.java:28)
	at middlelayer.MiddleLayer.send(MiddleLayer.java:79)
	at middlelayer.MiddleLayer.searchByIdentifier(MiddleLayer.java:277)
	at middlelayer.MiddleLayer.searchByIdentifier(MiddleLayer.java:261)
	at skipnode.SkipNode.insert(SkipNode.java:89)
	at unittest.LocalSkipGraph.insertAll(LocalSkipGraph.java:141)
	at skipnode.SkipNodeTest.testSearchByMembershipVectorSequential(SkipNodeTest.java:325)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:577)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:686)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
	at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:149)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:140)
	at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:84)
	at org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:104)
	at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:98)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$6(TestMethodTestDescriptor.java:205)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:201)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:137)
	at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:71)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:135)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:38)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$5(NodeTestTask.java:139)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$7(NodeTestTask.java:125)
	at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:135)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:123)
	at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:122)
	at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:80)
	at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:32)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
	at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:51)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:248)
	at org.junit.platform.launcher.core.DefaultLauncher.lambda$execute$5(DefaultLauncher.java:211)
	at org.junit.platform.launcher.core.DefaultLauncher.withInterceptedStreams(DefaultLauncher.java:226)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:199)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:132)
	at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:57)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
	at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)

[Logging] Putting instance attribute as thread context key value pairs

We can put instance variables such as name id, num id as thread context key-value pairs. By doing so, the logger can automatically add these variables into the log variables.

One catch is that if we do that output of some logs can be wrong for some test cases eg: concurrentInsertions
This is because the test case creates all skip node instances in one thread and then inserts them in separate threads.

If we do the above change, we should also modify such cases so that, we can avoid faulty logs

[Logging] migrate to log4j

Problem:

• The current code does not follow a strict and maintainable logging approach. We have lots of ad-hoc stdout printing all around the code, example here. This introduces the following problems:
• Logs are not exportable to any logging stream processor.
• Logs are not filterable at different levels.
• Logs are not parseable.

Proposed Solution:

Refactoring the code using a logger for logging parseable at different levels considering:

• Any stdout printing should be translated to debug level log, for example this.
• Any stderr printing should be translated to fatal level log, for example this. Also, the reports and stack traces should be included in the log.
• Logs should be parseable (e.g., using JSON format).
• We should not hard code any info rather than the plain message in the log, example of bad practice that hardcodes class name), rather we should rely on the logger to extract and include such info.
  💡 our suggest logging mechanism is log4j but any other logging mechanism is considerable.

Definition of Done:

• All direct printings on stdout and stderr (i.e., System. calls) are replaced with logging.
• Logs are parseable (e.g., in JSON format).
• No other logic of the code except logging is changed.

Development rules:

• Fork a branch in the form of yourname/issuenumber-issuetopic from master.
• Develop on the branch.
• Make sure that your code is well-commented, documented, supported by tests, and clean.
• The code should not add any commented functionality. If you comment something out, it means that you should completely remove it.
• Open the PR and make sure that it passes the pipeline.
• Important note: Your PR should not change more than 200 lines of code. If the surface of your PR is large, please break it into smaller ones, each completely passing the CI pipeline and able to merge, and open them one-by-one, i.e., open the first micro-PR, let it be reviewed and merge, and go with the second.

[Logging] Passing a single logger instance down the hierarchy

[Integration Tests] MVP

• To establish a decentralized (and not local) Skip Graph overlay of 32 nodes and test for full connectivity over each node, i.e., each node should be able to query every other node by both name and numerical IDs and get the correct response.

• The scenario should involve the first node creating itself through SkipNode constructor, then the rest of the nodes concurrently create themselves and try inserting in the skip graph using SkipNode.insert method by giving the first node as the introducer. **Note: for this we need to override another version of SkipNode constructor that does not require the LookupTable:

  public SkipNode(SkipNodeIdentity snID)
  

  We then run tests that each node queries a search for num ID of every other node concurrently and evaluate that the result is correct. We then do the same test for search for name ID.

• Note that in developing this issue we can borrow a lot of test logics from the already provided concurrentInsertion, searchByNameID,
  and searchByNumID. However, we need to be mindful that these tests are using LocalSkipGraph, however, we should have separate SkipNodes communicating with each other imitating a decentralized overlay. We can still keep the SkipNodes in an array or array list, but must not enforce any centralized behavior.

How to proceed?

• Create a branch <yourname>/<issue-number>-<issue-name>
• Develop a single mvpTest in /test/java/integation/mvpTest.java implementing the above scenario.
• The test should pass without making any change to the original implementation.

Definition of Done:

• Test implemented, passes, and the PR passes CI integration.

[Integration Test] DataNodeMVPTest

Test inspired by DataNodeTest, with the following differences:

• Instead of a LocalSkipGraph we use an actual skip graph similar to mvp test.
• We insert data nodes in the Skip Graph (concurrently each insertion in a separate thread).
• Instead of just checking for consistency, we search for inserted data nodes concurrently (each search in a thread), and see if the result is correct.

Refactor numerical ID to identifier type

Fixing broken tests

Context:

• Running mvn test as part of our CI was supposed to run all tests and fail CI if any test would fail. However, we noticed that it is currently not configured properly, and hence there are a handful of broken tests sitting on master branch.

• The purpose of this issue is to configure the maven plugins in the project properly so that any broken test would fail the CI. As the candidate, we suggest Surefire plugin of maven be adopted to our project.

• Once the plugging is set properly, the broken tests should be identified, their root cause investigated, and fixed one by one.

How to proceed?

• Create a branch <yourname>/<issue-number>-<issue-name>
• Make sure that you setup the Surefire plugin has setup correctly so that mvn test will _run all tests in src/test/java/** packages. Even develop a simple test and intentionally let it fail to verify whether Surefire can catch it properly.
• Identify all broken tests, start by the easiest one to fix, and fix it, and repeat this process iteratively till all tests are passed.

Definition of Done:

• Surefire plugin has been configured properly.
• mvn test will run all tests in specified package and will detect and catch any broken test.
• All broken tests have been identified and fixed one by one.