Skip to content
This repository has been archived by the owner on Apr 23, 2020. It is now read-only.

Add znode import and export support. #210

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adrianluisgonzalez
Copy link

Allow exporting and importing znodes via JSON files.

@qiangdavidliu
Copy link
Contributor

Hi @adrianluisgonzalez , thank you for the contribution. Unfortunately we are not yet ready to upgrade to guava 14 for exhibitor yet, are you able to rework your pull with the current guava version (11)? Thanks.

@adrianluisgonzalez
Copy link
Author

Hi @qiangdavidliu, the commit has been updated. Let me know if you have any other feedback.

@Randgalt
Copy link
Contributor

Randgalt commented Jan 7, 2015

I'll try to get to this soon. Thanks for your patience.

@adrianluisgonzalez
Copy link
Author

Hi @Randgalt, just wondering if you have any feedback.

@Randgalt
Copy link
Contributor

This is a large change. I can't promise when I'll get to it. But, I'll try.

@haad
Copy link

haad commented Jan 7, 2016

Can someone merge this ?

@Randgalt
Copy link
Contributor

Randgalt commented Jan 7, 2016

I'll try to look at this over the weekend.

@Randgalt
Copy link
Contributor

I like the idea of this, but it really should conform to Exhibitor's UI. It feels like it was just tacked on. If you don't have time to make the UI right, just remove it from the UI altogether and leave the REST API.

@@ -0,0 +1,75 @@
package com.netflix.exhibitor.core.rest;

import com.amazonaws.util.json.JSONArray;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not needed

@JohnWarnerEF
Copy link

Apologies for the UI side of things, it's not my strong suit. What changes to it would make it right for you?

@Randgalt
Copy link
Contributor

Look at how the other dialogs are done. It's pretty easy. I suck at UI.

@JohnWarnerEF
Copy link

So I've been looking at this further and memories have come rushing back. The problem is that the import function loads a file that the user specifies. I couldn't find a way of doing that with without having a form submission.

I didn't want to do that on the main page, as that seemed a poor user journey, so I created the popup window in order to do it.

There is a plugin called jquery-file-upload, which I may be able to get to work. But it looks like there will be version mismatch issues with JQuery UI, so that will likely have to have its version bumped.

So do you wish for us to just strip out the front-end code entirely, try this plugin or leave it as is, with the import being a separate window?

@Randgalt
Copy link
Contributor

I'd prefer to use the older jquery UI thing. It just looks odd to my eye otherwise.

@JohnWarnerEF
Copy link

At first I thought we could use an older version of jquery-file-upload in order to use an older version of JQuery UI. But even the oldest available version uses a later version of JQuery UI than is currently used by Exhibitor. So an upgrade would probably be required.

Also, currently we use a separate Jersey endpoint, ImportResource for the import, rather than ExplorerResource. This is in part because currently it is a separate page.

Would you like the import code merged into ExplorerResource?

@JohnWarnerEF
Copy link

Having looked at this some more, I remember now why there has to be a separate ImportResource, which is because of the way that all the other functions go through loading.html.

I have looked at trying to implement the front end using jquery-file-upload, but it's looking like it's going to be quite a large change that will involve changing a lot of the front end.

So I'm resubmitting the pull request with just the server-side code, including the changes you asked for, within it and all the front end changes removed.

@Randgalt
Copy link
Contributor

FYI - I'm in discussion with Netflix to move Exhibitor to its own repo so that it get better community support. Please stayed tuned.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants