-
Notifications
You must be signed in to change notification settings - Fork 475
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
KAA-1654: Add CoAP transport to Kaa #1361
base: master
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
@kaa-leeroy test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, Elnaz. Thank you for your pull request!
I've done a quick review for the code style compliance—see my inline comments. Please, ask if you need a help in fixing any of them.
@@ -0,0 +1,265 @@ | |||
package org.kaaproject.kaa.client.channel.impl.channels; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file needs Apache license header. Both Jenkins and Travis are currently failing because of missing Apache license headers.
* Date: 10/24/16 | ||
* Time: 10:52 AM | ||
* To change this template use File | Settings | File Templates. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove the auto-generated class header. (git already handles such info much better.)
private static final String CHANNEL_ID = "default_coap_channel"; | ||
private KaaDataDemultiplexer demultiplexer; | ||
private KaaDataMultiplexer multiplexer; | ||
// private volatile State channelState = State.CLOSED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code is not allowed.
It's always a code smell, you know. Weeks after the next guys does a refactoring and wonders what's the purpose of commented out code, why it is commented, should it be uncommented or deleted?
// if (channelState != State.OPENED) { | ||
// LOG.info("Can't sync. Channel [{}] is waiting for CONNACK message + KAASYNC message", getId()); | ||
// return; | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code (see my comment above)
// if (currentServer == null || socket == null) { | ||
// LOG.warn("Can't sync. Server is {}, socket is \"{}\"", currentServer, socket); | ||
// return; | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented out code (see my comment above)
* Date: 10/22/16 | ||
* Time: 3:36 PM | ||
* To change this template use File | Settings | File Templates. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above on auto-generated headers.
package org.kaaproject.kaa.server.transports.coap.transport; | ||
|
||
|
||
//import org.kaaproject.kaa.server.transport.; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code.
.getCommonProperties().getProperty(PUBLIC_INTERFACE_PROP_NAME, LOCALHOST))); | ||
|
||
|
||
//configuration.getBindInterface(), configuration.getBindPort() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code.
// create server | ||
server = new CoapHandler(configuration.getBindPort()); | ||
// add endpoints on all IP addresses | ||
//server.addEndpoints(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code.
server/transports/pom.xml
Outdated
@@ -36,5 +36,6 @@ | |||
<modules> | |||
<module>http</module> | |||
<module>tcp</module> | |||
<module>coap</module> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation level needs to be adjusted.
Hi, Alexey. |
@kaa-leeroy test this please |
1 similar comment
@kaa-leeroy test this please |
KAA-1640 has been fixed and the current Jenkins failure is caused by the changes in the PR. @elysoly could you please look at Jenkins logs to see what's wrong? |
Hi Alexye , |
I've created KAA-1654 for this implementation. |
Thank you guys , Should I close this PR? Is everything alright? |
@elysoly Hi! Please don't delete the PR: we have planned in a more in-depth code and architecture review into the current sprint. We will come back to you soon, likely with more suggestions on improvements, etc. Thanks for your patience while we review your PR. |
@kaa-leeroy test this please |
@kaa-leeroy test this please |
} | ||
|
||
/** | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we need some more java-docs with explanation. It would be better if all comments inside this method are moved to the java-doc .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the code is good. There are only some minor issues related to additional logs, java-docs and release versions of external libraries should be fixed.
However we have some failed tests on Jenkins that needs to be fixed as well. I believe there is some general problems with the tests, because main errors are: NPE, org.springframework.beans.factory.BeanCreationException and org.hibernate.service.UnknownServiceException.
The list of failed tests:
org.kaaproject.kaa.server.bootstrap.service.transport.BootstrapTransportServiceTest.messageHandlerProcessTest
org.kaaproject.kaa.server.control.ControlServerApplicationEventMapIT.testCreateApplicationEventFamilyMap
org.kaaproject.kaa.server.control.ControlServerApplicationEventMapIT.testGetVacantEventClassFamiliesByApplicationToken
org.kaaproject.kaa.server.control.ControlServerApplicationEventMapIT.testGetEventClassFamiliesByApplicationToken
org.kaaproject.kaa.server.control.ControlServerApplicationEventMapIT.testGetApplicationEventFamilyMap
org.kaaproject.kaa.server.control.ControlServerApplicationEventMapIT.testUpdateApplicationEventFamilyMap
org.kaaproject.kaa.server.control.ControlServerApplicationEventMapIT.testGetApplicationEventFamilyMapsByApplicationToken
org.kaaproject.kaa.server.control.ControlServerApplicationEventMapIT.org.kaaproject.kaa.server.control.ControlServerApplicationEventMapIT
org.kaaproject.kaa.server.control.ControlServerApplicationIT.org.kaaproject.kaa.server.control.ControlServerApplicationIT
org.kaaproject.kaa.server.control.ControlServerApplicationIT.org.kaaproject.kaa.server.control.ControlServerApplicationIT
org.kaaproject.kaa.server.control.ControlServerCTLSchemaIT.org.kaaproject.kaa.server.control.ControlServerCTLSchemaIT
org.kaaproject.kaa.server.control.ControlServerCTLSchemaIT.org.kaaproject.kaa.server.control.ControlServerCTLSchemaIT
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testGetConfigurationRecordBody
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testUpdateConfiguration
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testGetConfigurationRecord
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testGetConfigurationRecordsByEndpointGroupId
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testGetVacantSchemasByEndpointGroupId
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testActivateConfiguration
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testDeactivateConfiguration
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testDeleteConfigurationRecord
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.testCreateConfiguration
org.kaaproject.kaa.server.control.ControlServerConfigurationIT.org.kaaproject.kaa.server.control.ControlServerConfigurationIT
org.kaaproject.kaa.server.control.ControlServerConfigurationSchemaIT.org.kaaproject.kaa.server.control.ControlServerConfigurationSchemaIT
org.kaaproject.kaa.server.control.ControlServerConfigurationSchemaIT.org.kaaproject.kaa.server.control.ControlServerConfigurationSchemaIT
org.kaaproject.kaa.server.control.ControlServerEndpointGroupIT.testGetEndpointGroupsByApplicationToken
org.kaaproject.kaa.server.control.ControlServerEndpointGroupIT.testCreateEndpointGroup
org.kaaproject.kaa.server.control.ControlServerEndpointGroupIT.testGetEndpointGroup
org.kaaproject.kaa.server.control.ControlServerEndpointGroupIT.testDeleteEndpointGroup
org.kaaproject.kaa.server.control.ControlServerEndpointGroupIT.org.kaaproject.kaa.server.control.ControlServerEndpointGroupIT
org.kaaproject.kaa.server.control.ControlServerEventClassFamilyIT.org.kaaproject.kaa.server.control.ControlServerEventClassFamilyIT
org.kaaproject.kaa.server.control.ControlServerEventClassFamilyIT.org.kaaproject.kaa.server.control.ControlServerEventClassFamilyIT
org.kaaproject.kaa.server.control.ControlServerLogAppenderIT.org.kaaproject.kaa.server.control.ControlServerLogAppenderIT
org.kaaproject.kaa.server.control.ControlServerLogAppenderIT.org.kaaproject.kaa.server.control.ControlServerLogAppenderIT
org.kaaproject.kaa.server.control.ControlServerLogSchemaIT.getLogSchemaByApplicationTokenAndVersionTest
org.kaaproject.kaa.server.control.ControlServerLogSchemaIT.getLogSchemasByIdTest
org.kaaproject.kaa.server.control.ControlServerLogSchemaIT.getLogSchemaVersionsByApplicationTokenTest
org.kaaproject.kaa.server.control.ControlServerLogSchemaIT.getLogSchemasByApplicationTokenTest
org.kaaproject.kaa.server.control.ControlServerLogSchemaIT.org.kaaproject.kaa.server.control.ControlServerLogSchemaIT
org.kaaproject.kaa.server.control.ControlServerNotificationIT.org.kaaproject.kaa.server.control.ControlServerNotificationIT
org.kaaproject.kaa.server.control.ControlServerNotificationIT.org.kaaproject.kaa.server.control.ControlServerNotificationIT
org.kaaproject.kaa.server.control.ControlServerNotificationSchemaIT.testGetNotificationSchemasByAppToken
org.kaaproject.kaa.server.control.ControlServerNotificationSchemaIT.testGetNotificationSchemaVersionsByAppToken
org.kaaproject.kaa.server.control.ControlServerNotificationSchemaIT.testGetNotificationSchema
org.kaaproject.kaa.server.control.ControlServerNotificationSchemaIT.testGetUserNotificationSchemasByAppToken
org.kaaproject.kaa.server.control.ControlServerNotificationSchemaIT.testCreateNotificationSchema
org.kaaproject.kaa.server.control.ControlServerNotificationSchemaIT.org.kaaproject.kaa.server.control.ControlServerNotificationSchemaIT
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.testActivateProfileFilter
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.testDeactivateProfileFilter
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.testCreateProfileFilter
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.testDeleteProfileFilterRecord
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.testUpdateProfileFilter
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.testGetVacantSchemasByEndpointGroupId
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.testGetProfileFilterRecordsByEndpointGroupId
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.testGetProfileFilterRecord
org.kaaproject.kaa.server.control.ControlServerProfileFilterIT.org.kaaproject.kaa.server.control.ControlServerProfileFilterIT
org.kaaproject.kaa.server.control.ControlServerProfileSchemaIT.testGetProfileSchemasByApplicationToken
org.kaaproject.kaa.server.control.ControlServerProfileSchemaIT.testGetProfileSchemaVersionsByApplicationToken
org.kaaproject.kaa.server.control.ControlServerProfileSchemaIT.testCreateProfileSchema
org.kaaproject.kaa.server.control.ControlServerProfileSchemaIT.testGetProfileSchema
org.kaaproject.kaa.server.control.ControlServerProfileSchemaIT.testUpdateProfileSchema
org.kaaproject.kaa.server.control.ControlServerProfileSchemaIT.org.kaaproject.kaa.server.control.ControlServerProfileSchemaIT
org.kaaproject.kaa.server.control.ControlServerRecordLibraryIT.org.kaaproject.kaa.server.control.ControlServerRecordLibraryIT
org.kaaproject.kaa.server.control.ControlServerRecordLibraryIT.org.kaaproject.kaa.server.control.ControlServerRecordLibraryIT
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateCSdk
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateJavaSdkWithInvalidProfileSchema
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateJavaSdk
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateCppSdkWithEventSupport
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateCSdkWithEventSupport
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateJavaSdkWithEventSupport
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateJavaSdkWithInvalidApplication
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateCppSdk
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateAndroidSdkWithEventSupport
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateJavaSdkWithInvalidNotificationSchema
org.kaaproject.kaa.server.control.ControlServerSdkIT.testGenerateJavaSdkWithInvalidConfigurationSchema
org.kaaproject.kaa.server.control.ControlServerSdkIT.org.kaaproject.kaa.server.control.ControlServerSdkIT
org.kaaproject.kaa.server.control.ControlServerTenantIT.testCreateTenant
org.kaaproject.kaa.server.control.ControlServerTenantIT.testGetTenants
org.kaaproject.kaa.server.control.ControlServerTenantIT.testUpdateTenant
org.kaaproject.kaa.server.control.ControlServerTenantIT.testGetTenant
org.kaaproject.kaa.server.control.ControlServerTenantIT.org.kaaproject.kaa.server.control.ControlServerTenantIT
org.kaaproject.kaa.server.control.ControlServerTopicIT.org.kaaproject.kaa.server.control.ControlServerTopicIT
org.kaaproject.kaa.server.control.ControlServerTopicIT.org.kaaproject.kaa.server.control.ControlServerTopicIT
org.kaaproject.kaa.server.control.ControlServerUserIT.org.kaaproject.kaa.server.control.ControlServerUserIT
org.kaaproject.kaa.server.control.ControlServerUserIT.org.kaaproject.kaa.server.control.ControlServerUserIT
org.kaaproject.kaa.server.node.KaaNodeServerLauncherIT.testStartKaaNodeServerApplication
Also we would like to have a small documentation with some basic examples (like Data Collection Demo) and with simple tests.
<dependency> | ||
<groupId>org.eclipse.californium</groupId> | ||
<artifactId>californium-core</artifactId> | ||
<version>1.1.0-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid snapshot versions of external libraries.
Please use one of the release versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Californium version changed to 2.0.0-M2 .
server/transports/coap/pom.xml
Outdated
|
||
<properties> | ||
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
<main.dir>${basedir}/../../..</main.dir> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be better to use absolute path here, because the structure can be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relative path changed into absolute one.
|
||
<properties> | ||
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
<main.dir>${basedir}/../../../..</main.dir> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be an absolute path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes applied. Relative path changed into absolute one.
|
||
<dependency> | ||
<groupId>org.eclipse.californium</groupId> | ||
<artifactId>californium-core</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to have a release version. Also if it's used in several places it would be better to extract a property for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Californium version changed to 2.0.0-M2 .
/* | ||
* Definition of the Hello-World Resource | ||
*/ | ||
class HelloWorldResource extends CoapResource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some logs in each method to inform it's handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some logs info in each method.
* Initialize a transport instance with a particular configuration and | ||
* common transport properties that are accessible via the context. The configuration is an Avro | ||
* object. The serializaion/deserialization is done using the schema specified in | ||
* {@link KaaTransportConfig}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to have full name with package: @link org.kaaproject.kaa.server.transport.KaaTransportConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I've pushed the changes.
*/ | ||
@Override | ||
protected int getMinSupportedVersion() { | ||
// TODO Auto-generated method stub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to have some logs for each unfinished method to indicate that it returns always constant values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add SUPPORTED_VERSION as a constant for return.
add full name with package @link
Californium version changed to 2.0.0-M2 .
Dear @odovhai http://docs.kaaproject.org/display/KAA/Creating+custom+transport#Creatingcustomtransport-Transportprovisioning |
@elysoly |
Dear @odovhai , |
Dear @elysoly , Best Regards, |
Dear @odovhai |
Dear @elysoly , |
Hi, |
yes, Im Persian too. could you plz email it ?!?! mine is [email protected] |
I have been working on adding CoAP transport support to Kaa project. Now I pushed final results of my work. Hope to receive your comments about it.