Skip to content
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

use suro file's lastModify/dateCreated time for build upload path #147

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

iPinYou
Copy link
Contributor

@iPinYou iPinYou commented Oct 7, 2014

#140

configuration:
"prefixFormatter":{
"type":"FileDate",
"format":"yyyy/MM/dd/HH",
"dateType":"lastModify or dateCreated" //default dateCreated
},

@cloudbees-pull-request-builder

suro-pull-requests #115 SUCCESS
This pull request looks good

@@ -0,0 +1,42 @@
package com.netflix.suro.sink.remotefile.formatter;
Copy link
Contributor

Choose a reason for hiding this comment

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

I need a copyright

/*

  • Copyright 2014 Netflix, Inc.
    *
  • Licensed under the Apache License, Version 2.0 (the "License");
  • you may not use this file except in compliance with the License.
  • You may obtain a copy of the License at
  •    http://www.apache.org/licenses/LICENSE-2.0
    
  • Unless required by applicable law or agreed to in writing, software
  • distributed under the License is distributed on an "AS IS" BASIS,
  • WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  • See the License for the specific language governing permissions and
  • limitations under the License.
    */

@metacret
Copy link
Contributor

Overall, looks good to me except a few comments I mentioned above. I'd want to suggest instead of creating separate FileDatePrefixFormatter, what about adding new PrefixFormatter and integrating with DynamicRemotePrefixFormatter? Something like this

public static PrefixFormatter createFormatter(String formatString) {
        int startBracket = formatString.indexOf('(');
        int endBracket = formatString.lastIndexOf(')');

        String name = formatString.substring(0, startBracket);
        String param = formatString.substring(startBracket + 1, endBracket);

        if (name.equals("date")) {
            return new DatePrefixFormatter(param);
        } else if (name.equals("file_created_date")) {
            ...
        } else if (name.equals("file_lastmodified_date")) {
            ...
        } else if (name.equals("static")) {
            return new StaticPrefixFormatter(param);
        } else if (name.equals("property")) {
            return new PropertyPrefixFormatter(param);
        } else {
            throw new InvalidArgumentException(name + " cannot be supported");
        }
    }

DateRegionStackFormatter can be replaced by PropertyPrefixFormatter, and can be deprecated. So, extending DynamicRemotePrefixFormatter might be better option than creating a different one. What do you think?

@zhenchuan
Copy link

thanks for your review, i'll fixed them this weekend...

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

Successfully merging this pull request may close these issues.

5 participants