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
[WIP] Secure Dictionary Server Implementation #1152
Conversation
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2983/ |
Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/394/ |
retest this please |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2991/ |
Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/402/ |
retest this please |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2992/ |
Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/403/ |
d0aca3a
to
fa7d4b0
Compare
retest this please |
Build Failed with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/406/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2995/ |
Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/408/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2997/ |
fa7d4b0
to
f3693f3
Compare
Build Failed with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/417/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3006/ |
f3693f3
to
cf365df
Compare
Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/424/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3012/ |
@@ -57,7 +57,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception | |||
try { | |||
ByteBuf data = (ByteBuf) msg; | |||
DictionaryMessage key = new DictionaryMessage(); | |||
key.readData(data); | |||
key.readNonSecureData(data); |
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.
function can be named readskiplength
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
@@ -26,6 +26,7 @@ | |||
import org.apache.carbondata.core.metadata.schema.table.CarbonTable; | |||
import org.apache.carbondata.core.metadata.schema.table.column.CarbonDimension; | |||
|
|||
|
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.
remove extra line
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
@@ -50,7 +50,32 @@ | |||
*/ | |||
private DictionaryMessageType type; | |||
|
|||
public void readData(ByteBuf byteBuf) { | |||
public void readNonSecureData(ByteBuf byteBuf) { | |||
byteBuf.resetReaderIndex(); |
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.
remove this
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
// Configure the server. | ||
bindToPort(port); | ||
} | ||
public void startServer(SparkConf conf, String host, int port); |
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 interface should not contain SparkConf and hostname
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++; | ||
} | ||
} | ||
public void bindToPort(SparkConf orgConf, String host, int port); |
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.
only port is sufficient
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
key.setType(DictionaryMessageType.WRITE_DICTIONARY); | ||
dictionaryServerHandler.processMessage(key); | ||
} | ||
public String getSecretKey(); |
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 is common interface remove getSecretKey
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
nonSecureDictionaryServerHandler.processMessage(key); | ||
} | ||
|
||
@Override public String findLocalIpAddress() { |
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.
move all the common function to abstract class
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.
All cannot be moved as some of the class belongs to Spark2 and some in Core.
@@ -76,8 +75,7 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception | |||
* @param ctx | |||
* @param cause | |||
*/ | |||
@Override | |||
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { | |||
@Override public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { |
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.
revert this change
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
|
||
package org.apache.carbondata.core.dictionary.service; | ||
|
||
public class DictionaryOnePassService { |
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 class headers explaining purpose
this.port = port; | ||
} | ||
|
||
// @Override public DictionaryServer getDictionaryServer() { |
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.
use dictionary server provider to get instance
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.
Sending the server instance in provider is not needed.
@@ -59,7 +59,7 @@ class SerializableConfiguration(@transient var value: Configuration) extends Ser | |||
@transient | |||
private val LOGGER = LogServiceFactory.getLogService(this.getClass.getCanonicalName) | |||
|
|||
private def writeObject(out: ObjectOutputStream): Unit = | |||
private def writeObject(out: ObjectOutputStream): Unit = { |
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.
revert this change
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
DictionaryMessage dictionaryMessage; | ||
ByteBuffer resp = null; | ||
try { | ||
|
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.
clean buffers and data in finally block
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.
Handles. If response is Null the data will be handled.
ByteBuf buffer = ByteBufAllocator.DEFAULT.heapBuffer(); | ||
key.writeData(buffer); | ||
resp = client.sendRpcSync(buffer.nioBuffer(), 100000); | ||
} catch (Exception e) { |
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.
resp failure should be handled
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.
aLready Handled
return new OneForOneStreamManager(); | ||
} | ||
|
||
private static class DictionaryChannelFutureListener implements ChannelFutureListener { |
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.
remove this listener
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
*/ | ||
@Override | ||
public void startServer(SparkConf conf, String host, int port) { | ||
secureDictionaryServerHandler = new SecureDictionaryServerHandler(); |
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 configurations should be converted to TransportConf configurations
Secure Dictionary Implementation Along with Non Secure.
cf365df
to
28aa235
Compare
retest this please |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3047/ |
Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/459/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/3048/ |
Build Success with Spark 1.6, Please check CI http://144.76.159.231:8080/job/ApacheCarbonPRBuilder/460/ |
Can one of the admins verify this patch? |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/37/ |
SDV Build Fail , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1215/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/584/ |
Secure Dictionary Implementation Along with Non Secure.